Skip to content

Commit 3d3429f

Browse files
committed
[GitHub] Add workflows to manage merging of PRs from authors without commit access
These changes aim to make contributors and reviewers aware when a PR should be merged on the contributor's behalf. It happens in two parts: * Newly opened PRs will be labelled with "no-commit-access" if the author does not have commit access. * A new workflow will periodically check all open PRs with this label to see if: * There is at least one approval * No one is requesting changes * All checks are finished (not passed just finished, some failures can be explained) * If they do, it will remove the label and add a comment: * Instructing the author to make it merge ready, if needed. * Instructing approvers to merge it themselves, or to find someone who can. **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 Instead, we run the new workflow periodically. Checking Check status is done using PyGitHub's version of: https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#list-check-runs-for-a-git-reference There are some potential corner cases here, but the idea is that this is enough to make both author and reviewers aware that merge on behalf is an option. From there, I hope that they can communicate directly on the PR. If this does not happen in practice, we can revisit this and add more automation where it makes sense.
1 parent 17952b3 commit 3d3429f

File tree

3 files changed

+220
-0
lines changed

3 files changed

+220
-0
lines changed
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
name: "Find PRs That Need Merging on the Author's Behalf"
2+
3+
permissions:
4+
contents: read
5+
6+
on:
7+
schedule:
8+
# * is a special character in YAML so you have to quote this string
9+
# Run once an hour
10+
- cron: '5 * * * *'
11+
12+
jobs:
13+
check_needs_merge:
14+
runs-on: ubuntu-latest
15+
permissions:
16+
# May change labels and add a comment.
17+
pull-requests: write
18+
if: >-
19+
(github.repository == 'llvm/llvm-project')
20+
steps:
21+
- name: Checkout Automation Script
22+
uses: actions/checkout@v4
23+
with:
24+
sparse-checkout: llvm/utils/git/
25+
ref: main
26+
27+
- name: Setup Automation Script
28+
working-directory: ./llvm/utils/git/
29+
run: |
30+
pip install --require-hashes -r requirements.txt
31+
32+
- name: Check Open PRs
33+
working-directory: ./llvm/utils/git/
34+
run: |
35+
python3 ./github-automation.py \
36+
--token '${{ secrets.GITHUB_TOKEN }}' \
37+
check-prs-need-merge

.github/workflows/new-prs.yml

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,31 @@ jobs:
7373
# workaround for https://github.com/actions/labeler/issues/112
7474
sync-labels: ''
7575
repo-token: ${{ secrets.ISSUE_SUBSCRIBER_TOKEN }}
76+
77+
check-commit-access:
78+
runs-on: ubuntu-latest
79+
permissions:
80+
pull-requests: write
81+
if: >-
82+
(github.repository == 'llvm/llvm-project') &&
83+
(github.event.action == 'opened')
84+
steps:
85+
- name: Checkout Automation Script
86+
uses: actions/checkout@v4
87+
with:
88+
sparse-checkout: llvm/utils/git/
89+
ref: main
90+
91+
- name: Setup Automation Script
92+
working-directory: ./llvm/utils/git/
93+
run: |
94+
pip install --require-hashes -r requirements.txt
95+
96+
- name: Check Commit Access
97+
working-directory: ./llvm/utils/git/
98+
run: |
99+
python3 ./github-automation.py \
100+
--token '${{ secrets.GITHUB_TOKEN }}' \
101+
check-commit-access \
102+
--issue-number "${{ github.event.pull_request.number }}" \
103+
--author "${{ github.event.pull_request.user.login }}"

llvm/utils/git/github-automation.py

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,147 @@ def run(self) -> bool:
353353
return True
354354

355355

356+
"""
357+
CheckCommitAccess and CheckPRsNeedMerge work together to implement a flow that
358+
notifies approvers of PRs from authors without commit access, when an approved
359+
PR is ready to merge. The steps are as follows:
360+
361+
* Newly opened PRs from users without commit access are labelled to indicate that
362+
fact.
363+
* Periodically, PRs with this label are checked.
364+
* If they have approvals and all checks have finished, the author is prompted
365+
to check that the PR is merge ready and the approvers are prompted to merge
366+
it on their behalf if it is in a mergeable state.
367+
* The label is removed from the PR.
368+
369+
If we were able to write to the repo in response to a pull_request_review event,
370+
we could run the second part in response to the review submitted event.
371+
We cannot, so the second part runs on a timer instead. See:
372+
https://github.com/orgs/community/discussions/26651
373+
https://github.com/orgs/community/discussions/55940
374+
375+
"""
376+
377+
378+
def user_can_merge(user: str, repo):
379+
try:
380+
return repo.get_collaborator_permission(user) in ["admin", "write"]
381+
# There is a UnknownObjectException for this scenario, but this method
382+
# does not use it.
383+
except github.GithubException as e:
384+
# 404 means the author was not found in the collaborator list, so we
385+
# know they don't have push permissions. Anything else is a real API
386+
# issue, raise it so it is visible.
387+
if e.status != 404:
388+
raise e
389+
return False
390+
391+
392+
NO_COMMIT_ACCESS_LABEL = "no-commit-access"
393+
394+
395+
class CheckCommitAccess:
396+
def __init__(self, token: str, repo: str, pr_number: int, author: str):
397+
self.repo = github.Github(token).get_repo(repo)
398+
self.pr = self.repo.get_issue(pr_number).as_pull_request()
399+
self.author = author
400+
401+
def run(self) -> bool:
402+
if not user_can_merge(self.author, self.repo):
403+
self.pr.as_issue().add_to_labels(NO_COMMIT_ACCESS_LABEL)
404+
405+
return True
406+
407+
408+
class CheckPRsNeedMerge:
409+
PR_READY_COMMENT = "! This PR is ready to merge !"
410+
411+
def __init__(self, token: str, repo: str):
412+
self.repo = github.Github(token).get_repo(repo)
413+
414+
@staticmethod
415+
def at_users(users):
416+
return ", ".join([f"@{user.login}" for user in users])
417+
418+
def run(self) -> bool:
419+
# "Either open, closed, or all to filter by state." - no "approved"
420+
# unfortunately.
421+
for pull in self.repo.get_pulls(state="open"):
422+
if pull.base != "main":
423+
continue
424+
425+
for label in pull.as_issue().get_labels():
426+
if label.name == NO_COMMIT_ACCESS_LABEL:
427+
break
428+
else:
429+
# PR is from someone with commit access.
430+
continue
431+
432+
# Each reviewer may leave multiple reviews and could change their mind.
433+
# Find the most recent review for each user.
434+
reviews_by_user = {}
435+
for review in pull.get_reviews():
436+
if review.user in reviews_by_user:
437+
if review.sumitted_at > reviews_by_user[review.user].submitted_at:
438+
reviews_by_user[review.user] = review.state
439+
else:
440+
reviews_by_user[review.user] = review.state
441+
442+
# Looking for at least one approval, and no one asking for changes.
443+
approvers = []
444+
for user, state in reviews_by_user.items():
445+
if state == "APPROVED":
446+
approvers.append(user)
447+
elif state == "CHANGES_REQUESTED":
448+
approvers = []
449+
break
450+
451+
if not approvers:
452+
continue
453+
454+
# Wait for checks to finish before commenting.
455+
found_check_in_progress = False
456+
for commit in pull.get_commits():
457+
for run in commit.get_check_runs():
458+
if run.status in ["queued", "in_progress"]:
459+
found_check_in_progress = True
460+
break
461+
462+
if found_check_in_progress:
463+
continue
464+
465+
# Even approvers may not have commit access.
466+
can_merge = [a for a in approvers if user_can_merge(a, self.repo)]
467+
468+
if not can_merge:
469+
to_approvers = f"please find someone who can merge this PR on behalf of {self.at_users([pull.user])}"
470+
elif len(can_merge) == 1:
471+
to_approvers = (
472+
f"please merge this PR on behalf of {self.at_users([pull.user])}"
473+
)
474+
else:
475+
to_approvers = f"one of you should merge this PR on behalf of {self.at_users([pull.user])}"
476+
477+
mergers = can_merge if can_merge else approvers
478+
479+
pull.as_issue().create_comment(
480+
f"""\
481+
{self.at_users([pull.user])} please ensure that this PR is ready to be merged. Make sure that:
482+
* 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.
483+
* 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.
484+
485+
{self.at_users(mergers)}, check that:
486+
* The above items have been completed.
487+
* All checks have passed, or their failure has been adequately explained.
488+
489+
If so, {to_approvers}."""
490+
)
491+
492+
pull.as_issue().remove_from_labels(NO_COMMIT_ACCESS_LABEL)
493+
494+
return True
495+
496+
356497
def setup_llvmbot_git(git_dir="."):
357498
"""
358499
Configure the git repo in `git_dir` with the llvmbot account so
@@ -746,6 +887,12 @@ def request_release_note(token: str, repo_name: str, pr_number: int):
746887
pr_buildbot_information_parser.add_argument("--issue-number", type=int, required=True)
747888
pr_buildbot_information_parser.add_argument("--author", type=str, required=True)
748889

890+
check_commit_access_parser = subparsers.add_parser("check-commit-access")
891+
check_commit_access_parser.add_argument("--issue-number", type=int, required=True)
892+
check_commit_access_parser.add_argument("--author", type=str, required=True)
893+
894+
check_pr_needs_merge_parser = subparsers.add_parser("check-prs-need-merge")
895+
749896
release_workflow_parser = subparsers.add_parser("release-workflow")
750897
release_workflow_parser.add_argument(
751898
"--llvm-project-dir",
@@ -820,6 +967,14 @@ def request_release_note(token: str, repo_name: str, pr_number: int):
820967
args.token, args.repo, args.issue_number, args.author
821968
)
822969
pr_buildbot_information.run()
970+
elif args.command == "check-commit-access":
971+
check_commit_access = CheckCommitAccess(
972+
args.token, args.repo, args.issue_number, args.author
973+
)
974+
check_commit_access.run()
975+
elif args.command == "check-prs-need-merge":
976+
check_needs_merge = CheckPRsNeedMerge(args.token, args.repo)
977+
check_needs_merge.run()
823978
elif args.command == "release-workflow":
824979
release_workflow = ReleaseWorkflow(
825980
args.token,

0 commit comments

Comments
 (0)