[GitHub][workflows] Ask reviewers to merge new contributor's PRs #80659
Conversation
This is based on GitHub's example for this use case: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved When a review is submitted we check: * If it's an approval. * Whether it's on a PR from a new contributor (using the same logic as `new-prs.yml`). Then if we haven't already left a comment, add one to tell the author that they can have the PR merged by a reviewer. PRs can get multiple approvals, so we'll look for another magic HTML comment to tell if we've already done this. The comment doesn't ask the reviewers to merge it right away, just on the chance that the author still had things to do. As we don't have a norm of merging as soon as there is an approval, so doing that without asking might be surprising. It also notes that if we need multiple approvals to wait for those. Though in that situation I don't think GitHub will enable the merge button until they've all approved anyway. In future we can make this check more clever, perhaps by using a GitHub API to check all users and see if they have permissions. It's limited to new contributor PRs for now since we know for sure they won't have permissions. With this change, new contributor PRs will get: * A welcome comment on submission. * This merge on behalf comment on first approval. * A post merge comment about what to expect from the build bots.
|
@llvm/pr-subscribers-github-workflow Author: David Spickett (DavidSpickett) ChangesThis is based on GitHub's example for this use case: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved When a review is submitted we check:
Then if we haven't already left a comment, add one to tell the author that they can have the PR merged by a reviewer. PRs can get multiple approvals, so we'll look for another magic HTML comment to tell if we've already done this. The comment doesn't ask the reviewers to merge it right away, just on the chance that the author still had things to do. As we don't have a norm of merging as soon as there is an approval, so doing that without asking might be surprising. It also notes that if we need multiple approvals to wait for those. Though in that situation I don't think GitHub will enable the merge button until they've all approved anyway. In future we can make this check more clever, perhaps by using a GitHub API to check all users and see if they have permissions. It's limited to new contributor PRs for now since we know for sure they won't have permissions. With this change, new contributor PRs will get:
Full diff: https://github.com/llvm/llvm-project/pull/80659.diff 2 Files Affected:
diff --git a/.github/workflows/approved-prs.yml b/.github/workflows/approved-prs.yml
new file mode 100644
index 0000000000000..b8f911c87d9fc
--- /dev/null
+++ b/.github/workflows/approved-prs.yml
@@ -0,0 +1,47 @@
+name: "Prompt reviewers to merge PRs on behalf of new contributors."
+
+permissions:
+ contents: read
+
+on:
+ pull_request_review:
+ types:
+ - submitted
+
+jobs:
+ merge_on_behalf_information_comment:
+ runs-on: ubuntu-latest
+ permissions:
+ pull-requests: write
+ # If this PR was approved, and is authored by a new contributor. New
+ # contributor statuses don't work as expected, so we check that it is not
+ # any other status instead.
+ # (see https://github.com/orgs/community/discussions/78038)
+ if: >-
+ (github.repository == 'llvm/llvm-project') &&
+ (github.event.review.state == 'APPROVED') &&
+ (github.event.pull_request.author_association != 'COLLABORATOR') &&
+ (github.event.pull_request.author_association != 'CONTRIBUTOR') &&
+ (github.event.pull_request.author_association != 'MANNEQUIN') &&
+ (github.event.pull_request.author_association != 'MEMBER') &&
+ (github.event.pull_request.author_association != 'OWNER')
+ 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 -r requirements.txt
+
+ - name: Add Merge On Behalf Information Comment
+ working-directory: ./llvm/utils/git/
+ run: |
+ python3 ./github-automation.py \
+ --token '${{ secrets.GITHUB_TOKEN }}' \
+ pr-merge-on-behalf-information \
+ --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 04698cacbff92..a76fe4f01c0c3 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -298,6 +298,32 @@ def run(self) -> bool:
return True
+class PRMergeOnBehalfInformation:
+ COMMENT_TAG = "<!--LLVM MERGE ON BEHALF INFORMATION COMMENT-->\n"
+
+ def __init__(self, token: str, repo: str, pr_number: int, author: str):
+ repo = github.Github(token).get_repo(repo)
+ self.pr = repo.get_issue(pr_number).as_pull_request()
+ self.author = author
+
+ def run(self) -> bool:
+ # A review can be approved more than once, only comment the first time.
+ for comment in self.pr.as_issue().get_comments():
+ if self.COMMENT_TAG in comment.body:
+ return
+
+ # This text is using Markdown formatting.
+ comment = f"""\
+{self.COMMENT_TAG}
+@{self.author}, as a new contributor you do not have permissions to merge your own PRs yet.
+Please let us know when you are happy for this to be merged, and one of the reviewers can merge it on your behalf.
+
+(if many approvals are required, please wait until everyone has approved before merging)
+"""
+ self.pr.as_issue().create_comment(comment)
+ return True
+
+
def setup_llvmbot_git(git_dir="."):
"""
Configure the git repo in `git_dir` with the llvmbot account so
@@ -647,6 +673,14 @@ def execute_command(self) -> bool:
pr_buildbot_information_parser.add_argument("--issue-number", type=int, required=True)
pr_buildbot_information_parser.add_argument("--author", type=str, required=True)
+pr_merge_on_behalf_information_parser = subparsers.add_parser(
+ "pr-merge-on-behalf-information"
+)
+pr_merge_on_behalf_information_parser.add_argument(
+ "--issue-number", type=int, required=True
+)
+pr_merge_on_behalf_information_parser.add_argument("--author", type=str, required=True)
+
release_workflow_parser = subparsers.add_parser("release-workflow")
release_workflow_parser.add_argument(
"--llvm-project-dir",
@@ -700,6 +734,11 @@ def execute_command(self) -> bool:
args.token, args.repo, args.issue_number, args.author
)
pr_buildbot_information.run()
+elif args.command == "pr-merge-on-behalf-information":
+ pr_merge_on_behalf_information = PRMergeOnBehalfInformation(
+ args.token, args.repo, args.issue_number, args.author
+ )
+ pr_merge_on_behalf_information.run()
elif args.command == "release-workflow":
release_workflow = ReleaseWorkflow(
args.token,
|
|
This workflow will for certain miss people who are not new but don't have commit access, I'm aware of that limitation. My intent for now is to get the greeting - merge on behalf - post merge comments working for new PRs to complete the PR "lifecycle". Then after that look into something more precise like Graph QL queries to apply these sorts of workflows to any author. |
|
I've done this as a separate PR as it's quite different - #81142 |
|
Is it okay to close this PR given #81142 has superseded this? |
|
Sure, given that you're on board with the general idea for the other one. |
As suggested on an RFC for previous changes in this area - https://discourse.llvm.org/t/rfc-fyi-pull-request-greetings-for-new-contributors/75458/12.
This is based on GitHub's example for this use case: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved
When a review is submitted we check:
new-prs.yml).Then if we haven't already left a comment, add one to tell the author that they can have the PR merged by a reviewer. PRs can get multiple approvals, so we'll look for another magic HTML comment to tell if we've already done this.
The comment doesn't ask the reviewers to merge it right away, just on the chance that the author still had things to do. As we don't have a norm of merging as soon as there is an approval, so doing that without asking might be surprising.
It also notes that if we need multiple approvals to wait for those. Though in that situation I don't think GitHub will enable the merge button until they've all approved anyway.
In future we can make this check more clever, perhaps by using a GitHub API to check all users and see if they have permissions. It's limited to new contributor PRs for now since we know for sure they won't have permissions.
With this change, new contributor PRs will get: