New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 693093 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Last visit 27 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Sheriffbot-Auto-triage Rule: Merge Approval clean up

Project Member Reported by derat@chromium.org, Feb 16 2017

Issue description

Sheriffbot-Auto-triage Rule: Merge Approval clean up.


Add Bug number which got updated: issue 643661


Issue/Concern or feedback:

I saw a couple of browser crashes on a stable channel Chromebook running 55.0.2883.105 last night that look like they're caused by issue 643661, which was fixed back in early October. The fix was approved for merge to M55 on October 8th, but it looks like the merge never actually happened -- I'm guessing that it got lost in the shuffle.

On November 30, sheriffbot removed merge approval to M55 and commented: "This issue hasn't been updated in the last 6 weeks, so removing its merge approval label. Please re-request a merge if needed."

This scenario (or perhaps ones where a bug has a merge approval label for a particular milestone but never gets the corresponding "merge-merged-" label) seem like they should be manually reviewed, since we're shipping a bug that we previously believed was important enough to justify a merge.

So I think that my request is for SheriffBot to add a label indicating that these issues need manual review, and for someone (whoever's handling the branch/release?) to periodically do a scrub of them. Does that seem reasonable?
 

Comment 1 by laforge@google.com, Feb 16 2017

Cc: kerz@chromium.org amineer@chromium.org
Owner: cda...@chromium.org
Adding Alex and Jason to comment.

Given that this issue was in a Merge-Approved-55 state for 6 weeks and that there were 3 warnings (on the bug), this is something that should have been followed up in that 6 week period (i.e. it looks like there was a process breakdown).

Having a signal (e.g. flipping Merge-Approved-55 to Merge-Review-55) that something wasn't handled would help to raise process issues like this.  It likely wouldn't require a new label, unless we want to track how frequently it happens (e.g. Hotlist-Merge-Reivew-Unmerged).
Cc: cma...@chromium.org josa...@chromium.org
Sorry for late reply, finally getting to this as part of the great email backlog cleanup of Q1 2017.

+josafat@ since the bug in question was Chrome OS to get his thoughts.  +cmasso@ on iOS to see what process she and the iOS team follows on these bugs.

On the desktop and Android side, we use Chrome Dash to track merges across all states - you can see our instructions for triage and a sample of what that looks like here: https://docs.google.com/a/google.com/document/d/1DEi2qePmuu5_Xx38z4FvQBElHD6jxA8N65b4vZxG7Uw/edit?usp=sharing

For us, anything marked as Merge-Approved-## is labeled "Merge Pending" in the dashboard, which is triaged as follows: "These are merges that have been approved, but the developer has not yet actually performed the merge.  Please ping the bug (or ping the developer directly if more than a few days have passed) and request that the developer merge the change ASAP.  The sooner we include changes into our builds, the sooner we can detect any issues they may introduce, so please try to have all merges completed within five days."

We gate shipping a release while we have any items in the merge pending state, unless we have already reviewed them and don't think we need to block.  This is identical to how we handle release blockers in that it's a soft gate and relies on people to review prior to pushing a release.  I think we have enough training and eyes on this to ensure we're not going to miss anything in desktop and Android at this point (without putting super strict validation into our release tooling, which could introduce other issues).

Flipping the bug back to Merge-Review-## if untouched for some time is also possible, but we'd be relying on folks looking at those bugs just the same as we'd rely on folks looking at Merge-Approved-## so I don't know that sheriffbot can help here, this feels like more of an execution issue than anything.
I'd also point out the comment from the dev saying "sorry I missed the email approval" - so I think the discussions we've had re: incorporating merges into the weekly status updates, plus pointing more devs towards tools we have available (like Chrome / Chromium Dash, both of which inform developers of pending merges they need to take action on) will also help.

Comment 4 by cma...@chromium.org, Mar 23 2017

On iOS, it is the responsibility of the bug owner to make sure the fix gets merged into the release branch and so we don't have a "process" to track unmerged bugs. Also, testers verify that an RBS to be merged into a release branch does get merged since they test the fix on the release branch before marking the issue as verified. 

Comment 5 by cda...@chromium.org, May 10 2017

We have modified the merge-approval reminders to now (cc the merge-requester and the merge-approver) while sending the reminders (nags).

Comment 6 by cda...@chromium.org, Jul 24 2017

Owner: pras...@chromium.org
Status: Assigned (was: Untriaged)
Cc: -amineer@chromium.org
No longer on the Chrome team, e-mail me @google.com if any attention still required from me here, otherwise good luck!

Sign in to add a comment