[GitHub] Add workflow to check author's commit access on new PRs #123593
Conversation
This workflow will run on every opened PR and add a label if the author does not have the required permissions to merge their own PR. The permission check is based on code from llvm#81142, which tried to do this when a review was approved. That had to be reverted in llvm#81722 because the event that it was triggered by did not have permissions to write to the PR. So we have a slight disadvantage that a label could be wrong by the time the review is approved but ok, the author can click the button themselves then anyway. Plus, you could search by the label to find anything waiting for someone to merge on behalf.
|
@llvm/pr-subscribers-github-workflow Author: David Spickett (DavidSpickett) ChangesThis workflow will run on every opened PR and add a label if the author does not have the required permissions to merge their own PR. The permission check is based on code from #81142, which tried to do this when a review was approved. That had to be reverted in #81722 because the event that it was triggered by did not have permissions to write to the PR. So we have a slight disadvantage that a label could be wrong by the time the review is approved but ok, the author can click the button themselves then anyway. Plus, you could search by the label to find anything waiting for someone to merge on behalf. Full diff: https://github.com/llvm/llvm-project/pull/123593.diff 2 Files Affected:
diff --git a/.github/workflows/new-prs.yml b/.github/workflows/new-prs.yml
index 88175d6f8d64d4..344976b6be92c1 100644
--- a/.github/workflows/new-prs.yml
+++ b/.github/workflows/new-prs.yml
@@ -73,3 +73,31 @@ jobs:
# workaround for https://github.com/actions/labeler/issues/112
sync-labels: ''
repo-token: ${{ secrets.ISSUE_SUBSCRIBER_TOKEN }}
+
+ check-commit-access:
+ runs-on: ubuntu-latest
+ permissions:
+ pull-requests: write
+ if: >-
+ (github.repository == 'llvm/llvm-project') &&
+ (github.event.action == 'opened')
+ steps:
+ - name: Checkout Automation Script
+ uses: actions/checkout@v4
+ with:
+ sparse-checkout: llvm/utils/git/
+ ref: main
+
+ - name: Setup Automation Script
+ working-directory: ./llvm/utils/git/
+ run: |
+ pip install --require-hashes -r requirements.txt
+
+ - name: Check Commit Access
+ working-directory: ./llvm/utils/git/
+ run: |
+ python3 ./github-automation.py \
+ --token '${{ secrets.GITHUB_TOKEN }}' \
+ check-commit-access \
+ --issue-number "${{ github.event.pull_request.number }}" \
+ --author "${{ github.event.pull_request.user.login }}"
\ No newline at end of file
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index da467f46b4dd31..43d81e644e2024 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -290,6 +290,33 @@ def run(self) -> bool:
return True
+class CheckCommitAccess:
+ def __init__(
+ self, token: str, repo: str, pr_number: int, author: str):
+ self.repo = github.Github(token).get_repo(repo)
+ self.pr = self.repo.get_issue(pr_number).as_pull_request()
+ self.author = author
+
+ def can_merge(self, user: str) -> bool:
+ try:
+ return self.repo.get_collaborator_permission(user) in ["admin", "write"]
+ # There is a UnknownObjectException for this scenario, but this method
+ # does not use it.
+ except github.GithubException as e:
+ # 404 means the author was not found in the collaborator list, so we
+ # know they don't have push permissions. Anything else is a real API
+ # issue, raise it so it is visible.
+ if e.status != 404:
+ raise e
+ return False
+
+ def run(self) -> bool:
+ if not self.can_merge(self.author):
+ self.pr.as_issue().add_to_labels("no-commit-access")
+
+ return True
+
+
def setup_llvmbot_git(git_dir="."):
"""
Configure the git repo in `git_dir` with the llvmbot account so
@@ -680,6 +707,10 @@ def request_release_note(token: str, repo_name: str, pr_number: int):
pr_buildbot_information_parser.add_argument("--issue-number", type=int, required=True)
pr_buildbot_information_parser.add_argument("--author", type=str, required=True)
+check_commit_access_parser = subparsers.add_parser("check-commit-access")
+check_commit_access_parser.add_argument("--issue-number", type=int, required=True)
+check_commit_access_parser.add_argument("--author", type=str, required=True)
+
release_workflow_parser = subparsers.add_parser("release-workflow")
release_workflow_parser.add_argument(
"--llvm-project-dir",
@@ -751,6 +782,11 @@ def request_release_note(token: str, repo_name: str, pr_number: int):
args.token, args.repo, args.issue_number, args.author
)
pr_buildbot_information.run()
+elif args.command == "check-commit-access":
+ check_commit_access = CheckCommitAccess(
+ args.token, args.repo, args.issue_number, args.author
+ )
+ check_commit_access.run()
elif args.command == "release-workflow":
release_workflow = ReleaseWorkflow(
args.token,
|
|
Tested on my fork, where I added the label The label will need to be added by an admin before this can be merged. Edit: actually it appears I can add labels but regardless, we decide on a name first. |
DavidSpickett
requested review from
tstellar,
boomanaiden154,
jroelofs and
jplehr
|
I checked, and the labeller action we use for general labels cannot do this sort of check. Just file paths and branch names - https://github.com/actions/labeler?tab=readme-ov-file#match-object. |
|
✅ With the latest revision this PR passed the Python code formatter. |
|
Thank you for putting this together. This is amazing. |
|
Not an expert in github actions, but this looks ok to me. |
|
|
||
| def can_merge(self, user: str) -> bool: | ||
| try: | ||
| return self.repo.get_collaborator_permission(user) in ["admin", "write"] |
There was a problem hiding this comment.
This is probably plenty sufficient, but there's also the collaborators endpoint that enumerates explicit permissions like push: https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#list-repository-collaborators
It does however require paginating through the list so this is probably kinder to the rate limits
There was a problem hiding this comment.
I did not know about this, however I don't think we can use it here.https://pygithub.readthedocs.io/en/stable/github_objects/Repository.html?highlight=get_collaborator_permission#github.Repository.Repository.get_collaborators returns a list of all collaborators in the form of https://pygithub.readthedocs.io/en/stable/github_objects/NamedUser.html#github.NamedUser.NamedUser, which doesn't have a way to get to the permissions.
I see existing code on GitHub mainly using get_collaborators to get a list of usernames, then passing those one by one to get_collaborator_permission. I presume in workflows that cover the whole project not just one PR, here we just have them to check.
There was a problem hiding this comment.
Interesting that pygithub truncates so much of the response 🤔
I would say the current solution is good to go then.
This workflow will run on every opened PR and add a label if the author does not have the required permissions to merge their own PR.
The permission check is based on code from #81142, which tried to do this when a review was approved. That had to be reverted in #81722 because the event that it was triggered by did not have permissions to write to the PR.
So we have a slight disadvantage that a label could be wrong by the time the review is approved but ok, the author can click the button themselves then anyway.
Plus, you could search by the label to find anything waiting for someone to merge on behalf.
https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#get-repository-permissions-for-a-user is the GitHub API we end up calling and lists the possible values as "admin, write, read, and none".