Skip to content

Commit 1ec430a

Browse files
committed
[GitHub][workflows] Ask reviewers to merge PRs when author can not
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.
1 parent 46b6756 commit 1ec430a

File tree

2 files changed

+122
-0
lines changed

2 files changed

+122
-0
lines changed

.github/workflows/approved-prs.yml

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
name: "Prompt reviewers to merge PRs on behalf of authors"
2+
3+
permissions:
4+
contents: read
5+
6+
on:
7+
pull_request_review:
8+
types:
9+
- submitted
10+
11+
jobs:
12+
merge_on_behalf_information_comment:
13+
runs-on: ubuntu-latest
14+
permissions:
15+
pull-requests: write
16+
if: >-
17+
(github.repository == 'llvm/llvm-project') &&
18+
(github.event.review.state == 'APPROVED')
19+
steps:
20+
- name: Checkout Automation Script
21+
uses: actions/checkout@v4
22+
with:
23+
sparse-checkout: llvm/utils/git/
24+
ref: main
25+
26+
- name: Setup Automation Script
27+
working-directory: ./llvm/utils/git/
28+
run: |
29+
pip install -r requirements.txt
30+
31+
- name: Add Merge On Behalf Comment
32+
working-directory: ./llvm/utils/git/
33+
run: |
34+
python3 ./github-automation.py \
35+
--token '${{ secrets.GITHUB_TOKEN }}' \
36+
pr-merge-on-behalf-information \
37+
--issue-number "${{ github.event.pull_request.number }}" \
38+
--author "${{ github.event.pull_request.user.login }}"

llvm/utils/git/github-automation.py

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import argparse
1212
from git import Repo # type: ignore
1313
import html
14+
import json
1415
import github
1516
import os
1617
import re
@@ -298,6 +299,76 @@ def run(self) -> bool:
298299
return True
299300

300301

302+
class PRMergeOnBehalfInformation:
303+
COMMENT_TAG = "<!--LLVM MERGE ON BEHALF INFORMATION COMMENT-->\n"
304+
305+
def __init__(self, token: str, repo: str, pr_number: int, author: str):
306+
repo = github.Github(token).get_repo(repo)
307+
self.pr = repo.get_issue(pr_number).as_pull_request()
308+
self.author = author
309+
self.repo = repo
310+
self.token = token
311+
312+
def author_has_push_permission(self):
313+
# https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28#get-repository-permissions-for-a-user
314+
response = requests.get(
315+
# Where repo is "owner/repo-name".
316+
f"https://api.github.com/repos/{self.repo}/collaborators/{self.author}/permission",
317+
headers={
318+
"Accept": "application/vnd.github+json",
319+
"Authorization": f"Bearer {self.token}",
320+
"X-GitHub-Api-Version": "2022-11-28",
321+
},
322+
)
323+
324+
# 404 means this user is not a collaborator.
325+
if response.status_code == 404:
326+
# Does not have push permission if not a collaborator.
327+
return False
328+
# User is a collaborator.
329+
elif response.status_code == 200:
330+
user_details = json.loads(response.text)
331+
user = user_details["user"]
332+
333+
# We may have a list of permissions.
334+
if permissions := user.get("permissions"):
335+
return permissions["pull"]
336+
else:
337+
# Otherwise we can always fall back to the permission
338+
# on the top level object. The other permissions "read" and
339+
# "none" cannot push changes.
340+
return user_details["permisson"] in ["admin", "write"]
341+
else:
342+
# Something went wrong, log and carry on.
343+
print("Unexpected response code", response.status_code)
344+
# Assume they do have push permissions, so that we don't spam
345+
# PRs with comments if there are API problems.
346+
return True
347+
348+
def run(self) -> bool:
349+
# A review can be approved more than once, only comment the first time.
350+
# Doing this check first as I'm assuming we get the comment data "free" in
351+
# terms of API cost.
352+
for comment in self.pr.as_issue().get_comments():
353+
if self.COMMENT_TAG in comment.body:
354+
return
355+
356+
# Now check whether the author has permissions needed to merge, which
357+
# uses a REST API call.
358+
if self.author_has_push_permission():
359+
return
360+
361+
# This text is using Markdown formatting.
362+
comment = f"""\
363+
{self.COMMENT_TAG}
364+
@{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.
365+
366+
(if many approvals are required, please wait until everyone has approved before merging)
367+
"""
368+
self.pr.as_issue().create_comment(comment)
369+
return True
370+
371+
301372
def setup_llvmbot_git(git_dir="."):
302373
"""
303374
Configure the git repo in `git_dir` with the llvmbot account so
@@ -647,6 +718,14 @@ def execute_command(self) -> bool:
647718
pr_buildbot_information_parser.add_argument("--issue-number", type=int, required=True)
648719
pr_buildbot_information_parser.add_argument("--author", type=str, required=True)
649720

721+
pr_merge_on_behalf_information_parser = subparsers.add_parser(
722+
"pr-merge-on-behalf-information"
723+
)
724+
pr_merge_on_behalf_information_parser.add_argument(
725+
"--issue-number", type=int, required=True
726+
)
727+
pr_merge_on_behalf_information_parser.add_argument("--author", type=str, required=True)
728+
650729
release_workflow_parser = subparsers.add_parser("release-workflow")
651730
release_workflow_parser.add_argument(
652731
"--llvm-project-dir",
@@ -700,6 +779,11 @@ def execute_command(self) -> bool:
700779
args.token, args.repo, args.issue_number, args.author
701780
)
702781
pr_buildbot_information.run()
782+
elif args.command == "pr-merge-on-behalf-information":
783+
pr_merge_on_behalf_information = PRMergeOnBehalfInformation(
784+
args.token, args.repo, args.issue_number, args.author
785+
)
786+
pr_merge_on_behalf_information.run()
703787
elif args.command == "release-workflow":
704788
release_workflow = ReleaseWorkflow(
705789
args.token,

0 commit comments

Comments
 (0)