[llvm] [GitHub] Add workflows to manage merging of PRs from authors without commit access (PR #124910)
David Spickett via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 29 03:51:14 PST 2025
https://github.com/DavidSpickett created https://github.com/llvm/llvm-project/pull/124910
**Note:** This process could be simplified if we were able to write to the repository in response to the event generated when an approval is given. Due to security restrictions in GitHub, we cannot do this, see: https://github.com/orgs/community/discussions/26651 https://github.com/orgs/community/discussions/55940
I am introducing a pair of workflows that together do the following:
* Tell PR authors how to update their PR after approval, to make it ready for merging.
* Give them time to do so.
* Prompt PR approvers to merge the PR *only* once the PR is ready to merge. So all they have to do is click the button.
* All while not disturbing the workflow of anyone who *does* have commit access.
Due to the limitations mentioned above, the workflow is actually 2 workflows.
The first checks each opened PR and adds a "no-commit-access" label if the author has no comit access. I've implemented this as an addition to the existing workflow that runs on PR open.
A second workflow runs periodically and inspects all PRs that are open and have this label.
If the PR has approvals, a comment is added to tell the author how to make the PR ready for merging.
(content from https://llvm.org/docs/Contributing.html#how-to-submit-a-patch)
The author can take as much time as they like to do this, and when they are done, they reply with a comment with specific text to say so.
Note:
* I considered other things here, but comments is the only thing we can rely on a new contributor being able to add.
* I have handled the case where the author alreay knows the process and writes the magic comment ahead of time. I think it's valid to allow this as long as they're not obviously doing it to skip out on writing a decent PR description. Reviewers should be able to judge this.
When the next workflow run sees this comment it will add another comment, asking the approvers to merge the PR, and then it will remove the no-commit-access label. Removing the label makes the PR invisible to future runs of the workflow.
I have a demo PR on my fork: https://github.com/DavidSpickett/llvm-project/pull/105
(note that that ran with testing hacks because I am not a new user there)
This leaves the PR approved, ready to merge, with no extra labels. At this point approvers can just click the button, or if more approvals are required, they can wait and at least we know that it's in a decent state.
There is a corner case where you get 1 approval, and go through this process, but another reviewer vetos that approval. In this case, the label can be manually added and the bot comments and the user's magic comment removed, reseting the situation.
Maybe we can make this situation less manual, but it's going to get very complex very quickly so I haven't attempted to in this first version.
Ultimately, there'll always be some cases where we want to wait on more approvers. Even if we say "here is label you can add for a final approval", someone could add that and someone else could disagree. I think practical experience will be more informative than theory crafting here.
>From 4638523158ff023ff7cbfee5c3459c85a67a0d7e Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Mon, 20 Jan 2025 11:08:25 +0000
Subject: [PATCH] [GitHub] Add workflows to manage merging of PRs from authors
without commit access
**Note:** This process could be simplified if we were able to write to the
repository in response to the event generated when an approval is given.
Due to security restrictions in GitHub, we cannot do this, see:
https://github.com/orgs/community/discussions/26651
https://github.com/orgs/community/discussions/55940
I am introducing a pair of workflows that together do the following:
* Tell PR authors how to update their PR after approval,
to make it ready for merging.
* Give them time to do so.
* Prompt PR approvers to merge the PR *only* once the PR
is ready to merge. So all they have to do is click the button.
* All while not disturbing the workflow of anyone who *does*
have commit access.
Due to the limitations mentioned above, the workflow is actually
2 workflows.
The first checks each opened PR and adds a "no-commit-access"
label if the author has no comit access. I've implemented this
as an addition to the existing workflow that runs on PR open.
A second workflow runs periodically and inspects all PRs that
are open and have this label.
If the PR has approvals, a comment is added to tell the author
how to make the PR ready for merging.
(content from https://llvm.org/docs/Contributing.html#how-to-submit-a-patch)
The author can take as much time as they like to do this,
and when they are done, they reply with a comment with specific
text to say so.
Note:
* I considered other things here, but comments is the only thing
we can rely on a new contributor being able to add.
* I have handled the case where the author alreay knows the process
and writes the magic comment ahead of time. I think it's valid
to allow this as long as they're not obviously doing it to
skip out on writing a decent PR description. Reviewers should
be able to judge this.
When the next workflow run sees this comment it will add another
comment, asking the approvers to merge the PR, and then it will
remove the no-commit-access label. Removing the label makes the
PR invisible to future runs of the workflow.
I have a demo PR on my fork: https://github.com/DavidSpickett/llvm-project/pull/105
(note that that ran with testing hacks because I am not a new user there)
This leaves the PR approved, ready to merge, with no extra labels.
At this point approvers can just click the button, or if more approvals
are required, they can wait and at least we know that it's in a decent
state.
There is a corner case where you get 1 approval, and go through this
process, but another reviewer vetos that approval. In this case,
the label can be manually added and the bot comments and the user's
magic comment removed, reseting the situation.
Maybe we can make this situation less manual, but it's going to
get very complex very quickly so I haven't attempted to in this
first version.
Ultimately, there'll always be some cases where we want to wait
on more approvers. Even if we say "here is label you can add for
a final approval", someone could add that and someone else could
disagree. I think practical experience will be more informative
than theory crafting here.
---
.github/workflows/check-prs-need-merge.yml | 38 +++++
.github/workflows/new-prs.yml | 28 ++++
llvm/utils/git/github-automation.py | 160 +++++++++++++++++++++
3 files changed, 226 insertions(+)
create mode 100644 .github/workflows/check-prs-need-merge.yml
diff --git a/.github/workflows/check-prs-need-merge.yml b/.github/workflows/check-prs-need-merge.yml
new file mode 100644
index 00000000000000..eeba4cbd3b43a5
--- /dev/null
+++ b/.github/workflows/check-prs-need-merge.yml
@@ -0,0 +1,38 @@
+name: "Find PRs That Need Merging on the Author's Behalf"
+
+permissions:
+ contents: read
+
+on:
+ workflow_dispatch:
+ schedule:
+ # * is a special character in YAML so you have to quote this string
+ # Run once an hour
+ - cron: '5 * * * *'
+
+jobs:
+ check_needs_merge:
+ runs-on: ubuntu-latest
+ permissions:
+ # May change labels and add a comment.
+ pull-requests: write
+ if: >-
+ (github.repository == 'llvm/llvm-project')
+ steps:
+ - name: Checkout Automation Script
+ uses: actions/checkout at 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 Open PRs
+ working-directory: ./llvm/utils/git/
+ run: |
+ python3 ./github-automation.py \
+ --token '${{ secrets.GITHUB_TOKEN }}' \
+ check-prs-need-merge
diff --git a/.github/workflows/new-prs.yml b/.github/workflows/new-prs.yml
index 88175d6f8d64d4..bd8fe91c1e8ecc 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 at 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 }}"
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index da467f46b4dd31..20f9979c25fc77 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -290,6 +290,152 @@ def run(self) -> bool:
return True
+"""
+CheckCommitAccess and CheckPRsNeedMerge work together to implement a flow that
+notifies approvers of PRs from authors without commit access, when an approved
+PR is ready to merge. The steps are as follows:
+
+* New PRs from users without commit access are labelled to indicate this, as
+ they are opened.
+* Periodically, PRs with this tag are checked. If they have approvals, the author
+ is prompted to make the PR ready for merging.
+* The author then replies with a specific comment to say this has been done.
+* This comment is picked up by the next workflow run. We then add a comment prompting
+ the approvers of the PR to merge it, and remove the label we added earlier.
+
+We do allow for the author to post the magic comment earlier, and will skip prompting
+them if they have done that.
+
+We do this call and response so that:
+* Authors do not have to rush to update the PR.
+* The subsequent comment to approvers causes a notification to them which they
+ can immediately act upon.
+
+If we were able to write to the repo in response to a pull_request_review event,
+we could run the second part in response to the review submitted event. See:
+https://github.com/orgs/community/discussions/26651
+https://github.com/orgs/community/discussions/55940
+
+We cannot, so the second part runs on a timer instead.
+"""
+
+
+def user_can_merge(user: str, repo):
+ try:
+ return 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
+
+
+NO_COMMIT_ACCESS_LABEL = "no-commit-access"
+
+
+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 run(self) -> bool:
+ if not user_can_merge(self.author, self.repo):
+ self.pr.as_issue().add_to_labels(NO_COMMIT_ACCESS_LABEL)
+
+ return True
+
+
+class CheckPRsNeedMerge:
+ PROMPT_AUTHOR_COMMENT_TAG = "<!--NEEDS MERGE PROMPT AUTHOR-->\n"
+ PR_READY_COMMENT = "! This PR is ready to merge !"
+
+ def __init__(self, token: str, repo: str):
+ self.repo = github.Github(token).get_repo(repo)
+
+ @staticmethod
+ def at_users(users):
+ return ", ".join([f"@{user.login}" for user in users])
+
+ def prompt_author(self, pull) -> bool:
+ # Tell the PR author to prepare the PR for merge.
+ pull.as_issue().create_comment(
+ f"""\
+{self.PROMPT_AUTHOR_COMMENT_TAG}
+{self.at_users([pull.user])} please ensure that this PR is ready to be merged. Make sure that:
+* The PR title and description describe the final changes. These will be used as the title and message of the final squashed commit. The titles and messages of commits in the PR will **not** be used.
+* You have set a valid [email address](https://llvm.org/docs/DeveloperPolicy.html#github-email-address) in your GitHub account. This will be associated with this contribution.
+
+When the PR is ready to be merged please reply with a comment that is exactly "{self.PR_READY_COMMENT}"."""
+ )
+
+ return True
+
+ def prompt_approvers(self, pull, approvers) -> bool:
+ # Even approvers may not have commit access.
+ can_merge = [a for a in approvers if user_can_merge(a, self.repo)]
+
+ if not can_merge:
+ # If no one can merge, find someone who can.
+ to_approvers = f"{self.at_users(approvers)}, please find someone who can merge this PR on behalf of {at_users([pull.user])}."
+ elif len(can_merge) == 1:
+ # Ask this specific approver to merge.
+ to_approvers = f"{self.at_users(can_merge)}, please merge this PR on behalf of {self.at_users([pull.user])}."
+ else:
+ # Ask all who can merge to do so.
+ to_approvers = f"{self.at_users(can_merge)}, one of you should merge this PR on behalf of {self.at_users([pull.user])}."
+
+ pull.as_issue().create_comment(to_approvers)
+
+ return True
+
+ def run(self) -> bool:
+ # "Either open, closed, or all to filter by state." - no "approved"
+ # unfortunately.
+ for pull in self.repo.get_pulls(state="open"):
+ for label in pull.as_issue().get_labels():
+ if label.name == NO_COMMIT_ACCESS_LABEL:
+ break
+ else:
+ # PR is from someone with commit access.
+ continue
+
+ approvers = [r.user for r in pull.get_reviews() if r.state == "APPROVED"]
+ if not approvers:
+ # Can't do anything until there is at least one approval.
+ continue
+
+ found_prompt_author_comment = False
+ found_author_comment = False
+ for comment in pull.get_issue_comments():
+ # If the PR author wrote the magic response comment.
+ if (
+ comment.user.login == pull.user.login
+ and self.PR_READY_COMMENT in comment.body
+ ):
+ found_author_comment = True
+ # Either they responded to our prompting, or knew ahead of time
+ # what to do, either is fine.
+ break
+ elif self.PROMPT_AUTHOR_COMMENT_TAG in comment.body:
+ found_prompt_author_comment = True
+
+ if found_author_comment:
+ self.prompt_approvers(pull, approvers)
+ pull.as_issue().remove_from_labels(NO_COMMIT_ACCESS_LABEL)
+ elif not found_prompt_author_comment:
+ self.prompt_author(pull)
+ elif found_prompt_author_comment:
+ # Waiting for a response from the author.
+ pass
+
+ return True
+
+
def setup_llvmbot_git(git_dir="."):
"""
Configure the git repo in `git_dir` with the llvmbot account so
@@ -680,6 +826,12 @@ 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)
+
+check_pr_needs_merge_parser = subparsers.add_parser("check-prs-need-merge")
+
release_workflow_parser = subparsers.add_parser("release-workflow")
release_workflow_parser.add_argument(
"--llvm-project-dir",
@@ -751,6 +903,14 @@ 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 == "check-prs-need-merge":
+ check_needs_merge = CheckPRsNeedMerge(args.token, args.repo)
+ check_needs_merge.run()
elif args.command == "release-workflow":
release_workflow = ReleaseWorkflow(
args.token,
More information about the llvm-commits
mailing list