Uplift approval set in bugzilla without relman approval
Categories
(bugzilla.mozilla.org :: Phabricator Integration, defect)
Tracking
()
People
(Reporter: dmeehan, Assigned: dkl)
References
Details
Attachments
(1 file, 1 obsolete file)
Scenario:
- A phabricator revision was created targeting beta https://phabricator.services.mozilla.com/D248528
- The flag on the phabricator attachment in bugzilla was set to
approval-mozilla-beta?https://bugzilla.mozilla.org/show_bug.cgi?id=1962910#a976178_600971 - Someone outside of the relman reviewed the revision
- The relman reviewer group was added to the revision
What Happened:
- The flag on the phabricator attachment in bugzilla was set to
approval-mozilla-beta+https://bugzilla.mozilla.org/show_bug.cgi?id=1962910#a976178_600971 https://bugzilla.mozilla.org/show_bug.cgi?id=1962910#a1052125_600971
What's Expected:
- Only a review from the relman group should set the approval to
+
|
Assignee | |
Comment 1•5 months ago
|
||
Looking at this today.
|
|
Assignee | |
Comment 2•5 months ago
|
||
Seems to be in order.
- nrishel accepted the revision on Thu, May 8, 7:11 PM.
- the approval flag was not set as nrishel does not have permission to set the flag to +.
- dmeehan added a reviewer: release-managers on Fri, May 9, 1:58 PM
- The approval-mozilla-beta+ flag was added by phab-bot on the bug right after since it saw that the revision was in an 'accepted' state.
- The approval flag was set because dmeehan does have permission to set the + value.
So it looks like the code is working as intended unless we have the workflow wrong all this time.
|
Reporter | |
Comment 3•5 months ago
|
||
:dkl, I'm not sure I follow Comment 2, maybe my steps in Comment 0 were misleading.
"dmeehan added a reviewer: release-managers on Fri, May 9, 1:58 PM" This is correct, we can see I added the relman reviewer group. This was just so it was ready for whenever it was reviewed.
However, we can see I never reviewed the revision then. It was only reviewed, aka "uplift approved", when RyanVM reviewed it on Tue, May 13
|
|
Assignee | |
Comment 4•5 months ago
|
||
(In reply to Donal Meehan [:dmeehan] from comment #3)
:dkl, I'm not sure I follow Comment 2, maybe my steps in Comment 0 were misleading.
"dmeehan added a reviewer: release-managers on Fri, May 9, 1:58 PM" This is correct, we can see I added the relman reviewer group. This was just so it was ready for whenever it was reviewed.
However, we can see I never reviewed the revision then. It was only reviewed, aka "uplift approved", when RyanVM reviewed it on Tue, May 13
No I didn't say you reviewed it, but that you made a change to the revision after it was prior reviewed by someone else. It was prior accepted by nrishel. Since firefox-beta is tagged as an uplift repository, then the PhabBugz code attempts to set an approval flag. In the case of nrishel, he did not have the permissions to set the flag so it was skipped. Then you made a different change right after, the PhabBugz code again saw that the revision was accepted and that the repo has the uplift tag. And again it tried to set the approval flag and this time you had the proper permissions.
The PhabBugz code has no idea what "uplift approved" is and to it, RyanVM was just a second reviewer that accepted the revision.
Then you made a different change right after, the PhabBugz code again saw that the revision was accepted and that the repo has the uplift tag.
The revision wasn't accepted by anyone in the RelMan group, so PhabBugz shouldn't have treated the revision as accepted.
In other words the approval flag must only be set to + if the revision has been accepted by a member of the RelMan group. Other reviews related to the code itself, and not authority to land.
|
|
Reporter | |
Comment 6•5 months ago
|
||
The PhabBugz code has no idea what "uplift approved" is and to it
Sorry, I only mentioned "aka uplift approved" as the time when the review should have set the approval flag to +.
I understand there is no concept of "uplift approved", only logic around approval flags changing when someone in the relman group accepts the review.
|
|
Assignee | |
Comment 7•5 months ago
|
||
(In reply to :glob ✱ from comment #5)
Then you made a different change right after, the PhabBugz code again saw that the revision was accepted and that the repo has the uplift tag.
The revision wasn't accepted by anyone in the RelMan group, so PhabBugz shouldn't have treated the revision as accepted.
In other words the approval flag must only be set to + if the revision has been accepted by a member of the RelMan group. Other reviews related to the code itself, and not authority to land.
Thank you for the clarification. It is currently not coded this way in the PhabBugz extension. Apologize if that was a requirement that was missed. This logic and code has been this way for quite a while. Right now it is not looking at who accepted the revision, only that is accepted and that the most recent changer has permissions to move the flag to a granted state. In the case of the approval flag, it is members of the mozilla-next-drivers group.
If we want to change the code to explicitly look up the members of the #release-managers group and then do look at the list of reviewers who accepted the revision to make sure one or more members are the ones who accepted, we will need to do some refactoring and other changes to support that. The work will need to be scheduled.
|
||
Comment 8•4 months ago
|
||
Authored by https://github.com/dklawren
https://github.com/mozilla-bteam/bmo/commit/3eb010c5e1b4d2478beccb7cb5e0b4826acd005a
[master] Bug 1965553 - Uplift approval set in bugzilla without relman approval (#2441)
|
|
||
Comment 9•4 months ago
|
||
Authored by https://github.com/dklawren
https://github.com/mozilla-bteam/bmo/commit/93041a7568c0ba5826bb4db438e475ae5ee3ce4d
[master] Revert "Bug 1965553 - Uplift approval set in bugzilla without relman approval (#2441)"
|
|
Assignee | |
Comment 10•4 months ago
•
|
||
Reopening. Caused the following error in the logs:
{"Type":"Bugzilla.Extension.PhabBugz.Feed","Timestamp":1748365392000000000,"Pid":7,"Severity":6,"EnvVersion":2.0,"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Fields":{"msg":"REVISION CHANGE FOUND: D251367: Bug 1968483 - turn off
http3 in getHSTSPreloadList.js. r?#necko | bug: 1968483 | jcristau | jcristau created D251367: Bug 1968483 - turn off http3 in getHSTSPreloadList.js. r?#necko.","type":"FEED"}}
{"Type":"Bugzilla.Extension.PhabBugz.Feed","Pid":7,"Timestamp":1748365392000000000,"Severity":6,"EnvVersion":2.0,"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Fields":{"msg":"Bug is public so setting view/edit public","type":"FEE
D"}}
{"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Fields":{"type":"FEED","msg":"Checking for revision attachment"},"Type":"Bugzilla.Extension.PhabBugz.Feed","Pid":7,"Timestamp":1748365393000000000,"EnvVersion":2.0,"Severity":6}
{"Fields":{"type":"FEED","msg":"New attachment 9490988 created"},"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Severity":6,"EnvVersion":2.0,"Timestamp":1748365393000000000,"Pid":7,"Type":"Bugzilla.Extension.PhabBugz.Util"}
{"Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Logger":"STDERR","Fields":{"msg":"Updating obsolete status on attachmment 9490988 to 0","type":"FEED"},"Severity":6,"EnvVersion":2.0,"Type":"Bugzilla.Extension.PhabBugz.Feed","Timestamp":1748365393000
000000,"Pid":7}
{"EnvVersion":2.0,"Severity":6,"Timestamp":1748365393000000000,"Pid":7,"Type":"Bugzilla.Extension.PhabBugz.Feed","Fields":{"msg":"If uplift repository we may need to set approval flags","type":"FEED"},"Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","
Logger":"STDERR"}
{"Fields":{"type":"FEED","msg":"Uplift repository detected. Setting attachment approval flags"},"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","EnvVersion":2.0,"Severity":6,"Pid":7,"Timestamp":1748365393000000000,"Type":"Bugzilla.E
xtension.PhabBugz.Feed"}
{"Fields":{"msg":"Can't call method \"id\" on unblessed reference at /app/extensions/PhabBugz/lib/Util.pm line 72.\n"},"Logger":"STDERR","Hostname":"gha-bugzilla-phabbugz-55b44b6f78-gg8db","Severity":0,"EnvVersion":2.0,"Timestamp":1748365393000000000,"Pi
d":7,"Type":"Bugzilla.Extension.PhabBugz.Feed"}
|
|
||
Comment 11•4 months ago
|
||
|
|
||
Comment 12•4 months ago
|
||
Authored by https://github.com/dklawren
https://github.com/mozilla-bteam/bmo/commit/d83421901cbbf9e2f8598680ee8a219798753e5a
[master] Bug 1965553 - Uplift approval set in bugzilla without relman approval
|
|
||
Comment 13•4 months ago
|
||
Authored by https://github.com/dklawren
https://github.com/mozilla-bteam/bmo/commit/dc049229a29dd60b80cd4d159831af69c151d782
[master] Revert "Bug 1965553 - Uplift approval set in bugzilla without relman approval"
|
|
Assignee | |
Comment 14•4 months ago
|
||
Reopening again :(
The issue I see so far is that we are now only looking at actual reviews and nothing else. When a new revision is added and the repo is an uplift repo, there are no reviews yet. If no reviews, then we exit the function. But the approval-mozilla-beta? flag still needs to be set and not skipped when the revision is created. So a special case needs to be added for that.
Working on a fix
|
|
||
Comment 15•4 months ago
|
||
|
|
Assignee | |
Updated•4 months ago
|
|
|
||
Comment 16•4 months ago
|
||
Authored by https://github.com/dklawren
https://github.com/mozilla-bteam/bmo/commit/06d1029d973815bbd0ed800bb2fc51d0bab47ab2
[master] Bug 1965553 - Uplift approval set in bugzilla without relman approval
Description
•