1046827 - Add the ability to request approval at bug (rather than patch) granularity

Status: RESOLVED WONTFIX

(bugzilla.mozilla.org :: Administration, task)

Description

[Bobby Holley (:bholley)]

See bug 1022229 comment 49.

Comment 1

[:glob ✱]

this is a very major change to the approval policy, and needs approval from all teams that use the current per-attachment flag.

bholly: have you cleared this with all users of that flag?

Comment 2

[:glob ✱]

i suspect this is a duplicate of bug 103605

Comment 3

[Bobby Holley (:bholley)]

(In reply to Byron Jones ‹:glob› from comment #1)
> this is a very major change to the approval policy

Hm, why? Presumably we could just set it up so that the approval request appears on an invisible placeholder patch called "all the patches for this bug" or something. If some team didn't like that, they could just deny the request.

I can certainly send an email to dev-platform and see if there's consensus, but trying to clear this with all of the bajillion b2g teams is a lot of stop-energy.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Byron Jones ‹:glob› from comment #2)
> i suspect this is a duplicate of bug 103605

Also, it isn't, because it doesn't handle the edge cases of additional patches (and followup bustages fixes) that didn't make it to bugzilla.

Comment 5

[:glob ✱]

(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Byron Jones ‹:glob› from comment #1)
> > this is a very major change to the approval policy
> 
> Hm, why? Presumably we could just set it up so that the approval request
> appears on an invisible placeholder patch called "all the patches for this
> bug" or something. If some team didn't like that, they could just deny the
> request.

(In reply to Bobby Holley (:bholley) from comment #4)
> Also, it isn't, because it doesn't handle the edge cases of additional
> patches (and followup bustages fixes) that didn't make it to bugzilla.

these comments are contradictory.

if you want to allow approving of patches "that didn't make it to bugzilla", then the only way to implement that is to change the approval flags from being per-attachment to per-bug.  without that change there won't be anywhere to store the flag status.  as this changes how approvals are managed and tracked it needs buy-in from all impacted parties.

if you want to just expose the approval flag on the bug page so all attachments can be updated at once, that's another way of expressing bug 103605.


given tracking of patches that aren't present in bugzilla is an edge case and the impact changing the approval flag will have, fixing bug 103605 appears to be the right thing to do.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Byron Jones ‹:glob› from comment #5)
> these comments are contradictory.

Um, what? Why?
 
> if you want to allow approving of patches "that didn't make it to bugzilla",
> then the only way to implement that is to change the approval flags from
> being per-attachment to per-bug.  without that change there won't be
> anywhere to store the flag status. as this changes how approvals are
> managed and tracked it needs buy-in from all impacted parties.

My suggestion was to make the flag per-bug, but to have it appear in everybody's existing request-queues as a patch called "ALL_PATCHES_FOR_THIS_BUG" or somesuch. This means that nobody's queries or workflow needs to change, so I'm not convinced it needs exhaustive approval (though a discussion on dev-platform would certainly be a good idea).

> if you want to just expose the approval flag on the bug page so all
> attachments can be updated at once, that's another way of expressing bug
> 103605.

It's not, because of the case of patches not on bugzilla. See below.

> given tracking of patches that aren't present in bugzilla is an edge case
> and the impact changing the approval flag will have, fixing bug 103605
> appears to be the right thing to do.

No it's not - it happens all the time. Specifically, every time there's an in-place followup for compilation or test bustage, or additional tests landed, or "r=me with additional patch to rename X to Y". If we move to a system where individual patches in the bug are batch-flagged for approval, it puts the sheriff in the role of either second-guessing the flags on bugzilla, or uplifting the wrong code to branches.

In general, we should avoid having multiple sets of state so that there's no confusion over which state is canonical.

Comment 7

[:glob ✱]

(In reply to Bobby Holley (:bholley) from comment #6)
> My suggestion was to make the flag per-bug, but to have it appear in
> everybody's existing request-queues as a patch called
> "ALL_PATCHES_FOR_THIS_BUG" or somesuch. This means that nobody's queries or
> workflow needs to change, so I'm not convinced it needs exhaustive approval
> (though a discussion on dev-platform would certainly be a good idea).

flags are either per-bug or per-attachment.  we cannot make bug-level flags present themselves as attachment flags without behind the scenes hacks which morph requests or inject fake flags.  that isn't a solution that i'm willing to entertain as it be complex to implement correctly and will likely cause issues.

changing the flag will have an impact on systems which interface with bmo (dashboards, metrics, etc), and your proposal _is_ a major change and cannot happen without buy-in from all impacted parties (firefox, thunderbird, seamonkey, nss, nspr, etc).


> > given tracking of patches that aren't present in bugzilla is an edge case
> > and the impact changing the approval flag will have, fixing bug 103605
> > appears to be the right thing to do.
> 
> No it's not - it happens all the time.

in comment 4 you said that it's an "edge case"; i was echoing your diagnosis.

> In general, we should avoid having multiple sets of state so that there's no
> confusion over which state is canonical.

i don't doubt there's an issue, however this proposed solution is non-trivial work which impacts more than just bugzilla itself.

given 1022229 comment 50 (from a sherrif) states "at least for now, what we've been doing seems to work well, so I'm not overly worried about it", changing the approval flag to bug-level will be more trouble than it's worth.