Approve a PR by leaving "/lgtm" comment #153
Conversation
Signed-off-by: lugi0 <[email protected]>
for more information, see https://pre-commit.ci
Signed-off-by: lugi0 <[email protected]>
Signed-off-by: lugi0 <[email protected]>
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/verified', '/hold', '/lgtm'} |
Signed-off-by: lugi0 <[email protected]>
Signed-off-by: lugi0 <[email protected]>
|
see lugi0#2 for testing of the workflow |
.github/workflows/lgtm-approval.yml
Outdated
| on: | ||
| workflow_dispatch: #manual testing | ||
| issue_comment: | ||
| types: [created] |
There was a problem hiding this comment.
please add condition to trigger only if "lgtm in comment"
e/g
if: |
contains(github.event.comment.body, '/lgtm')
also should be triggered on edited
There was a problem hiding this comment.
The actions already checks if /lgtm is int he comment: https://github.com/opendatahub-io/opendatahub-tests/pull/153/files#diff-8adeafbcbe79cc05a3ae753fd95a88451697a3356e53b468f8a7f565c5c737d1R13
Why would we edit a comment to add a lgtm? that seems unlikely to be honest
There was a problem hiding this comment.
- i think adding the check on the content of the comment at the top makes it more clear
- a user can edit a comment and add "lgtm"
There was a problem hiding this comment.
I've added the edit event, I still think it is not a likely scenario though :)
The if conditional is not supported on the definition of the event as far as I can see.
Signed-off-by: lugi0 <[email protected]>
.github/workflows/lgtm-approval.yml
Outdated
| permissions: | ||
| pull-requests: write | ||
| if: > | ||
| (github.event.issue.pull_request && contains(github.event.comment.body, '/lgtm')) || |
There was a problem hiding this comment.
what if i use /lgtm cancel or delete my comment? add and remove pr approval should be done in our python code
There was a problem hiding this comment.
Done in python code, now both options work
|
|
||
| def dismiss_pr_approval(self) -> None: | ||
| all_reviews = self.pr.get_reviews() | ||
| current_user = self.gh_client.get_user().login |
There was a problem hiding this comment.
isn't this self.user_login?
There was a problem hiding this comment.
No, the approval through the action is left by the bot, so we need to dismiss the bot's review.
We could expand it to also remove for the user who has left the comment if an approval by them exists, but we run into the issue that we don't have a token for each user and we won't be able to dismiss it using the bot's token
| if label_in_pr: | ||
| LOGGER.info(f"Removing label {label}") | ||
| self.pr.remove_from_labels(label=label) | ||
|
|
||
| elif not label_in_pr: | ||
| self.add_pr_label(label=label) | ||
| self.approve_pr() |
There was a problem hiding this comment.
wouldn't this approave the pr for all user labels?
pr should be approved only if self.user_login != pr owner
There was a problem hiding this comment.
wouldn't this approave the pr for all user labels?
shoot, yeah, I was sure it was indented under the first if block, will fix tomorrow
pr should be approved only if self.user_login != pr owner
let's discuss about this, I think there are more scenarios to consider
There was a problem hiding this comment.
Checking for team membership now. I've done a workaround until we can get the bot into the org but the logic is there.
| @@ -31,19 +36,41 @@ jobs: | |||
| comment-id: ${{ github.event.comment.id }} | |||
| reactions: '+1' | |||
|
|
|||
| # This currently fails with either the bot PAT or the standard github token secret | |||
There was a problem hiding this comment.
Do you want to remove these?
| allowed_users = self.get_allowed_users() | ||
| if self.user_login not in allowed_users: | ||
| LOGGER.info(f"User {self.user_login} is not allowed for this action. Exiting.") | ||
| sys.exit(0) |
There was a problem hiding this comment.
Should you exit or add a comment? Exiting would just fail the action.
Description
This add a GHA to approve a PR when a user leaves a "/lgtm" comment
How Has This Been Tested?
Merge criteria: