Skip to content

Commit f95e442

Browse files
author
Release Manager
committed
gh-36213: Fix sync labels issues for step 2 going live <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes #1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes #12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> This is a follow up PR of #35172. It addresses the issues observed in the second step of going live (#35927 (comment)) of the label sync bot, which had to be disabled after a day (#35927 (comment)). As observed in #36177 (comment), this was caused by a bug concerning the `reviewDecision` in case the reviewer does not have appropriate access rights to the repository. In this case `gh pr view` returns None as `reviewDecision` which has been mapped to `COMMENTED` by caching reasons. The bug is that comparisons with `reviewDecision` have not been adapted to the cached value. Furthermore, the log-file sequence displayed in #36177 (comment) shows another unexpected behavior: ``` > INFO:root:Proper reviews after 2023-09-02T07:55:03Z for pull request #36177: [{'id': 'PRR_kwDOI5-Tx85f2_5f', 'author': {'login': 'dcoudert'}, 'authorAssociation': 'CONTRIBUTOR', 'body': 'LGTM.', 'submittedAt': '2023-09-02T13:23:57Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}, {'id': 'PRR_kwDOI5-Tx85f3sRb', 'author': {'login': 'tobiasdiez'}, 'authorAssociation': 'CONTRIBUTOR', 'body': "Let's see if its works when admins approve a PR", 'submittedAt': '2023-09-03T06:08:30Z', 'includesCreatedEdit': False, 'reactionGroups': [], 'state': 'APPROVED', 'commit': {'oid': '626b764283b5005ab3a64661f7cc74efcd31adb9'}}] ``` Here I had expected `'authorAssociation': 'MEMBER'` instead of `'authorAssociation': 'CONTRIBUTOR'` as it is shown when I do the query on my local command-line: ``` gh pr view #36177 --json reviews { "reviews": [ { "author": { "login": "dcoudert" }, "authorAssociation": "MEMBER", "body": "LGTM.", "submittedAt": "2023-09-02T13:23:57Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" }, { "author": { "login": "tobiasdiez" }, "authorAssociation": "MEMBER", "body": "Let's see if its works when admins approve a PR", "submittedAt": "2023-09-03T06:08:30Z", "includesCreatedEdit": false, "reactionGroups": [], "state": "APPROVED" } ] } ``` As described in flutter/flutter#101012 this is caused since the github bot is not a member of the sagemath organisation and most of our members (me too) have set the private role which makes their membership just visible to other members. This is relevant in the `approve_allowed` method: ``` def approve_allowed(self): r""" Return if the actor has permission to approve this PR. """ revs = self.get_reviews(complete=True) if not any(rev['authorAssociation'] in ('MEMBER', 'OWNER') for rev in revs): info('PR %s can\'t be approved because of missing member review' % (self._issue)) return False .... ``` where the bot checks if it should set the `approved` state by itself. This did also fail in the example (from the [log-file](https://github.co m/sagemath/sage/actions/runs/6060223064/job/16444158489)): ``` INFO:root:PR pull request #36177 doesn't have positve review (by decision) INFO:root:PR pull request #36177 can't be approved because of missing member review ``` I think we can eliminate the check for a member review entirely as it is an unnecessary restriction. The check is only relevant if an `s: positive review` label is added, which can only be done by Triage members who know what they are doing. Even if we just added `CONTRIBUTOR` to the list, it would still be restrictive, as it would not be possible to set `s: positive review` when there is no review (this would be annoying for trivial PR that just has some PEP8 fixes). ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - #12345: short description why this is a dependency - #34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: #36213 Reported by: Sebastian Oehms Reviewer(s): Tobias Diez
2 parents e74579d + 50b91da commit f95e442

File tree

1 file changed

+6
-8
lines changed

1 file changed

+6
-8
lines changed

.github/sync_labels.py

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class ReviewDecision(Enum):
5656
"""
5757
changes_requested = 'CHANGES_REQUESTED'
5858
approved = 'APPROVED'
59-
unclear = 'COMMENTED'
59+
unclear = 'UNCLEAR'
6060

6161
class Priority(Enum):
6262
r"""
@@ -319,12 +319,15 @@ def get_review_decision(self):
319319
return None
320320

321321
if self._review_decision is not None:
322+
if self._review_decision == ReviewDecision.unclear:
323+
return None
322324
return self._review_decision
323325

324326
data = self.view('reviewDecision')
325327
if data:
326328
self._review_decision = ReviewDecision(data)
327329
else:
330+
# To separate a not supplied value from not cached (see https://github.com/sagemath/sage/pull/36177#issuecomment-1704022893 ff)
328331
self._review_decision = ReviewDecision.unclear
329332
info('Review decision for %s: %s' % (self._issue, self._review_decision.value))
330333
return self._review_decision
@@ -349,9 +352,9 @@ def get_reviews(self, complete=False):
349352
self.get_commits()
350353

351354
date = self._commit_date
352-
no_rev = ReviewDecision.unclear.value
355+
unproper_rev = RevState.commented.value
353356
new_revs = [rev for rev in self._reviews if rev['submittedAt'] > date]
354-
proper_new_revs = [rev for rev in new_revs if rev['state'] != no_rev]
357+
proper_new_revs = [rev for rev in new_revs if rev['state'] != unproper_rev]
355358
info('Proper reviews after %s for %s: %s' % (date, self._issue, proper_new_revs))
356359
return proper_new_revs
357360

@@ -463,11 +466,6 @@ def approve_allowed(self):
463466
r"""
464467
Return if the actor has permission to approve this PR.
465468
"""
466-
revs = self.get_reviews(complete=True)
467-
if not any(rev['authorAssociation'] in ('MEMBER', 'OWNER') for rev in revs):
468-
info('PR %s can\'t be approved because of missing member review' % (self._issue))
469-
return False
470-
471469
revs = self.get_reviews()
472470
revs = [rev for rev in revs if rev['author']['login'] != self._actor]
473471
ch_req = ReviewDecision.changes_requested

0 commit comments

Comments
 (0)