[GitHub][workflows] Ask reviewers to merge PRs when author cannot

[DavidSpickett]

This uses https://pygithub.readthedocs.io/en/stable/github_objects/Repository.html?highlight=get_collaborator_permission#github.Repository.Repository.get_collaborator_permission.

Which does https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#get-repository-permissions-for-a-user and returns the top level "permission" key.

This is less detailed than the user/permissions key but should be fine for this
use case.

When a review is submitted we check:

• If it's an approval.
• Whether we have already left a merge on behalf comment (by looking for a hidden HTML comment).
• Whether the author has permissions to merge their own PR.

If needed we leave a comment tagging the reviewer. If the reviewer also doesn't have merge permission, then it asks them to find someone else who does.

[llvmbot]

@llvm/pr-subscribers-github-workflow

Author: David Spickett (DavidSpickett)

Changes

This is based on GitHub's examples:
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#running-a-workflow-when-a-pull-request-is-approved https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#get-repository-permissions-for-a-user

When a review is submitted we check:

• If it's an approval.
• Whether we have already left a merge on behalf comment (by looking for a hidden HTML comment).
• Whether the author has permissions to merge their own PR (using a REST API call).

The comment doesn't ask the reviewers to merge it right away, just in case 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.

GitHub does have limits for the REST API:
https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28

And I've made some rough assumptions based on there being 37900 commits in the tree Jan 2023 to Jan 2024. If we assumed every one of those was a PR (they weren't) that would be roughly 4 per hour.

I'm not sure if llvm would be using the personal rate: "All of these requests count towards your personal rate limit of 5,000 requests per hour." Or the higher enterprise rate:
"Requests made on your behalf by a GitHub App that is owned by a GitHub Enterprise Cloud organization have a higher rate limit of 15,000 requests per hour."

If we assume the lower limit, that's 5000 approvals per hour. Assuming ~2 approval events per PR that's 2500 per hour we can run this job on.

There are secondary limits too.

"No more than 100 concurrent requests are allowed."

Seems unlikely we would hit this given that we'd have to have 100 approval events that managed to get scheduled at the exact same time on the runners.

"No more than 900 points per minute are allowed for REST API endpoints"

The request here is 1 point, so we'd have to have 900 approval events in one minute, not likely.

"In general, no more than 80 content-generating requests per minute and no more than 500 content-generating requests per hour are allowed."

We are only reading permissions, so this isn't an issue. Leaving the comment, the majority of PRs won't need a comment anyway.

In case of issues with the API I have written the check to assume the author has permission should anything go wrong. This means we default to not leaving any comments.

---

Full diff: https://github.com/llvm/llvm-project/pull/81142.diff

2 Files Affected:

• (added) .github/workflows/approved-prs.yml (+38)
• (modified) llvm/utils/git/github-automation.py (+84)

diff --git a/.github/workflows/approved-prs.yml b/.github/workflows/approved-prs.yml
new file mode 100644
index 0000000000000..6a037f7aead0e
--- /dev/null
+++ b/.github/workflows/approved-prs.yml
@@ -0,0 +1,38 @@
+name: "Prompt reviewers to merge PRs on behalf of authors"
+
+permissions:
+  contents: read
+
+on:
+  pull_request_review:
+    types:
+      - submitted
+
+jobs:
+  merge_on_behalf_information_comment:
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write
+    if: >-
+      (github.repository == 'llvm/llvm-project') &&
+      (github.event.review.state == 'APPROVED')
+    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 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..50475916144c6 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -11,6 +11,7 @@
 import argparse
 from git import Repo  # type: ignore
 import html
+import json
 import github
 import os
 import re
@@ -298,6 +299,76 @@ 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
+        self.repo = repo
+        self.token = token
+
+    def author_has_push_permission(self):
+        # https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#get-repository-permissions-for-a-user
+        response = requests.get(
+            # Where repo is "owner/repo-name".
+            f"https://api.github.com/repos/{self.repo}/collaborators/{self.author}/permission",
+            headers={
+                "Accept": "application/vnd.github+json",
+                "Authorization": f"Bearer {self.token}",
+                "X-GitHub-Api-Version": "2022-11-28",
+            },
+        )
+
+        # 404 means this user is not a collaborator.
+        if response.status_code == 404:
+            # Does not have push permission if not a collaborator.
+            return False
+        # User is a collaborator.
+        elif response.status_code == 200:
+            user_details = json.loads(response.text)
+            user = user_details["user"]
+
+            # We may have a list of permissions.
+            if permissions := user.get("permissions"):
+                return permissions["pull"]
+            else:
+                # Otherwise we can always fall back to the permission
+                # on the top level object. The other permissions "read" and
+                # "none" cannot push changes.
+                return user_details["permisson"] in ["admin", "write"]
+        else:
+            # Something went wrong, log and carry on.
+            print("Unexpected response code", response.status_code)
+            # Assume they do have push permissions, so that we don't spam
+            # PRs with comments if there are API problems.
+            return True
+
+    def run(self) -> bool:
+        # A review can be approved more than once, only comment the first time.
+        # Doing this check first as I'm assuming we get the comment data "free" in
+        # terms of API cost.
+        for comment in self.pr.as_issue().get_comments():
+            if self.COMMENT_TAG in comment.body:
+                return
+
+        # Now check whether the author has permissions needed to merge, which
+        # uses a REST API call.
+        if self.author_has_push_permission():
+            return
+
+        # This text is using Markdown formatting.
+        comment = f"""\
+{self.COMMENT_TAG}
+@{self.author}, 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 +718,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 +779,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,

[boomanaiden154]

No major issues on my end. I'm wondering if at some point we want to split github-automation.py given it serves quite a few functions at this point that don't seem to overlap super well. That's a separate discussion/PR though.

[DavidSpickett]

Updated this to use PyGitHub instead.

[DavidSpickett]

https://github.com/PyGithub/PyGithub/pull/881/files if you want to see the details of the API call.

[tstellar]

I think this message should be directed at the reviewer, not the author.

[DavidSpickett]

Last update is broken because I only tested it with myself, we need the author and reviewer name. Will update shortly.

[DavidSpickett]

Fixed. We now pass the PR author (to check for merge permissions) and review author (to address the comment to) to the automation script.

It's possible the reviewer doesn't have permissions either. We can check this too but I'm wondering what would be useful to say in that scenario. Perhaps @foo the author of this PR does not have permission to merge PRs, and neither do you. Please find someone who can merge on the author's behalf..

Then the obvious follow up is how do they find someone to merge it, discord is probably the best place?

We could even edit the comment if we later got an approval from someone who does have merge permission, but trying not to complicate this too much.

[tstellar]

Can the default GITHUB_TOKEN @mention people?

[tstellar]

I think it can @mention people but not teams so this should be fine.

[DavidSpickett]

On my fork it was able to @ mention me at least (but I could have different settings so that could be why).

[tstellar]

I think it can @mention people but not teams so this should be fine.

[boomanaiden154]

No issues on my end.

[DavidSpickett]

I modified the comment for when the reviewer also lacks permissions, what do you think @tstellar ?

Discord seems the obvious place to find people online at the time, or they can @ another reviewer to have them do it.

[DavidSpickett]

CI is failing because it's running the workflow from my fork, but it gets the script from main which doesn't have the new action of course. This is expected.

Which seemed a bit scary to me, but going off of https://github.blog/2023-08-09-four-tips-to-keep-your-github-actions-workflows-secure/ "An important security guarantee that GitHub makes is that workflows triggered on pull_request from forks are run with minimal privileges: no access to secrets and the repository token is read-only. " - the workflow wouldn't have been able to post a comment anyway.

[DavidSpickett]

Windows failure also unrelated.