Skip to content

Conversation

DavidSpickett
Copy link
Collaborator

Reverts #81142

Turns out that when using the pull_request_review event, you cannot have write permissions for GITHUB_TOKEN. So this workflow is always going to fail with:

github.GithubException.GithubException: 403 {"message": "Resource not accessible by integration", "documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment"}

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review

See also:
https://github.com/orgs/community/discussions/26651
https://github.com/orgs/community/discussions/55940

So we will have to find another trigger for this, that is able to add comments.

I think this worked while testing on my fork because the PR was from one branch of my fork to another of that same fork.

@llvmbot
Copy link
Member

llvmbot commented Feb 14, 2024

@llvm/pr-subscribers-github-workflow

Author: David Spickett (DavidSpickett)

Changes

Reverts llvm/llvm-project#81142

Turns out that when using the pull_request_review event, you cannot have write permissions for GITHUB_TOKEN. So this workflow is always going to fail with:

github.GithubException.GithubException: 403 {"message": "Resource not accessible by integration", "documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment"}

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_review

See also:
https://github.com/orgs/community/discussions/26651
https://github.com/orgs/community/discussions/55940

So we will have to find another trigger for this, that is able to add comments.

I think this worked while testing on my fork because the PR was from one branch of my fork to another of that same fork.


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

2 Files Affected:

  • (removed) .github/workflows/approved-prs.yml (-39)
  • (modified) llvm/utils/git/github-automation.py (-65)
diff --git a/.github/workflows/approved-prs.yml b/.github/workflows/approved-prs.yml
deleted file mode 100644
index 309a9217e42d31..00000000000000
--- a/.github/workflows/approved-prs.yml
+++ /dev/null
@@ -1,39 +0,0 @@
-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 }}" \
-            --reviewer "${{ github.event.review.user.login }}"
diff --git a/llvm/utils/git/github-automation.py b/llvm/utils/git/github-automation.py
index ccef274c4c1f7c..b475eff06fc3eb 100755
--- a/llvm/utils/git/github-automation.py
+++ b/llvm/utils/git/github-automation.py
@@ -298,55 +298,6 @@ 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, reviewer: str
-    ):
-        self.repo = github.Github(token).get_repo(repo)
-        self.pr = self.repo.get_issue(pr_number).as_pull_request()
-        self.author = author
-        self.reviewer = reviewer
-
-    def can_merge(self, user: str) -> bool:
-        try:
-            return self.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
-
-    def run(self) -> bool:
-        # Check this first because it only costs 1 API point.
-        if self.can_merge(self.author):
-            return
-
-        # 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.
-        if self.can_merge(self.reviewer):
-            comment = f"""\
-{self.COMMENT_TAG}
-@{self.reviewer} the PR author does not have permission to merge their own PRs yet. Please merge on their behalf."""
-        else:
-            comment = f"""\
-{self.COMMENT_TAG}
-@{self.reviewer} the author of this PR does not have permission to merge and neither do you.
-Please find someone who has merge permissions who can merge it on the author's behalf. This could be one of the other reviewers or you can ask on [Discord](https://discord.com/invite/xS7Z362)."""
-
-        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
@@ -714,17 +665,6 @@ 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)
-pr_merge_on_behalf_information_parser.add_argument(
-    "--reviewer", type=str, required=True
-)
-
 release_workflow_parser = subparsers.add_parser("release-workflow")
 release_workflow_parser.add_argument(
     "--llvm-project-dir",
@@ -784,11 +724,6 @@ 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, args.reviewer
-    )
-    pr_merge_on_behalf_information.run()
 elif args.command == "release-workflow":
     release_workflow = ReleaseWorkflow(
         args.token,

@DavidSpickett
Copy link
Collaborator Author

Already done by 124cd11.

@nunoplopes nunoplopes deleted the revert-81142-no-push-approved branch December 28, 2024 09:56
DavidSpickett added a commit to DavidSpickett/llvm-project that referenced this pull request Jan 20, 2025
This workflow will run on every opened PR and add a label if the
author does not have the required permissions to merge their own
PR.

The permission check is based on code from llvm#81142,
which tried to do this when a review was approved. That had to be
reverted in llvm#81722 because
the event that it was triggered by did not have permissions to write
to the PR.

So we have a slight disadvantage that a label could be wrong by
the time the review is approved but ok, the author can click
the button themselves then anyway.

Plus, you could search by the label to find anything waiting
for someone to merge on behalf.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants