1191015 - No way to carry "Ship It!" (with nits) forward when commit changed, spamming reviewers.

Status: RESOLVED INVALID

(MozReview Graveyard :: Review Board: Extension, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The discretion of reviewers to trust level 1 and 2 requesters to follow-up and comply, without bothering the reviewer again, has effectively been removed, to the extent mozReview is the new tool to use.

This is the "r+ with comments addressed" workflow from Bug 1169360 comment 1. This trust-based workflow has AFAIK delivered for several teams for years in Bugzilla + Splinter, regardless of people's push access-level.

For those teams, the change-over to using mozReview leads to reviewers being spammed with requests, even when they've said they don't need to be bothered again, whereas before with splinter this was not a problem. As such the experience is a regression in workflow.

Bug 1175166 admits that this is a desirable workflow for some, by enabling it only when the requester has level 3 access, but this seems to leave others in a lurch.

It would also be nice to get an explanation for the reuse of push access level for this new purpose, when on the surface it seems perhaps a bit unintuitive that people with low access levels are somehow a higher-risk category in mozReview, when they already can't land things without the help of others.

Here's an example: https://reviewboard.mozilla.org/r/14429/diff/2-3/
See Bug 1189030 comment 10 and 11 for the spam.

Workaround:

The workaround is for the reviewer to mark the requests with r+ manually in Bugzilla, which seems accepted by everyone involved, but this fights the tool and spams yet again, and leads to wondering what the point is.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> Here's an example:

Bad example since this had zero changes which is Bug 1169360, but you get the idea.

Comment 2

[Mark Côté [:mcote]]

A point of clarification, and apologies for my ignorance: how did L1/L2 users land "r+ with comments addressed" patches before?  Did they upload a new patch without any reviewers and have the reviewer or a sheriff land it?  If so, would not the r+ be lost, since it would belong to the obsoleted patch?

I ask because the reasoning behind only allowing L3 users to carry forward r+s is that they are allowed to land patches regardless, so they can be trusted to update a patch without further review.  I'm unclear how that applies to people who cannot land on their own.

I feel like I'm missing something here that could inform a better process in MozRevew.

(This also presumes we are talking about mozilla-central and other repos that require L3 to land; if we're talking about other, less restrictive repos, let me know.)

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Mark Côté [:mcote] from comment #2)
> A point of clarification, and apologies for my ignorance: how did L1/L2
> users land "r+ with comments addressed" patches before?  Did they upload a
> new patch without any reviewers and have the reviewer or a sheriff land it?

Pretty much, except I
1. put r=jesup in the commit msg of the patch,
2. mark the bugzilla attachment as r+ myself,
3. write a comment e.g. "Nits addressed. Carrying forward r=jesup."
4. set keyword: checkin-needed
5. wait for someone to land it.

> If so, would not the r+ be lost, since it would belong to the obsoleted
> patch?

It'll say jib:r+

I like putting r=jesup in the patch's description as well, so the relevant reviewer(s) is still visible.

> I ask because the reasoning behind only allowing L3 users to carry forward
> r+s is that they are allowed to land patches regardless, so they can be
> trusted to update a patch without further review.

The trust isn't from their ability to land patches. They're allowed to land patches because they are trusted (to not mess that up).

> I'm unclear how that applies to people who cannot land on their own.

L1/L2 users are also trusted. They're trusted to set checkin-needed on self-r+'ed patches, but not trusted to land these themselves on m-i without messing something up.

I was told not everyone needs L3 access, as it scales poorly, but this is making me doubt that.

Comment 4

[Steven MacLeod [:smacleod]]

I think something missing from this conversation is where all this is leading and how that has affected our decisions about level access. Autoland to inbound is planned for this quarter which will cutout manual
pushes of things reviewed in MozReview. While this means a L3 user won't actually have to push the code, we still want to tie into the level system to gate landings because it's the trust model we have atm.

Autoland will refuse to land a change to inbound unless one of the following is met:
- The requests author is L3, and has received a ship-it from *anyone*. (indicating a review has happened - I could see it argued this requirement should be dropped for L3 though)
- The requests author is < L3, and has received a ship-it from at least one L3 reviewer on the version of the commit to be landed.

I believe this model decently reflects the level system, and puts the burden of landing and trust decisions on the set of L3 users (While it's not perfect, as L2 users may land code in some trees, our primary focus is m-c atm).

So, when autoland is turned on, when a L2 or below user posts a new version of code an L3 user *must* ship it that latest version before it can land. I'm uncomfortable allowing code to land in a different way without a larger discussion involving module owners where this code could land.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> (In reply to Mark Côté [:mcote] from comment #2)
> > A point of clarification, and apologies for my ignorance: how did L1/L2
> > users land "r+ with comments addressed" patches before?  Did they upload a
> > new patch without any reviewers and have the reviewer or a sheriff land it?
> 
> Pretty much, except I
> 1. put r=jesup in the commit msg of the patch,
> 2. mark the bugzilla attachment as r+ myself,
> 3. write a comment e.g. "Nits addressed. Carrying forward r=jesup."
> 4. set keyword: checkin-needed
> 5. wait for someone to land it.
> 
> > If so, would not the r+ be lost, since it would belong to the obsoleted
> > patch?
> 
> It'll say jib:r+
> 
> I like putting r=jesup in the patch's description as well, so the relevant
> reviewer(s) is still visible.

The issue here is that unless whoever is landing things actually checks the new patch and verifies the author only changed the nits noted by the reviewer it is possible that broken code (whether intentional or not) is present in the new patch. This is particularly problematic for level 1 which is quite easy to attain and would allow slipping security vulnerabilities into updated patches.

 
> L1/L2 users are also trusted. They're trusted to set checkin-needed on
> self-r+'ed patches, but not trusted to land these themselves on m-i without
> messing something up.

I've never heard before that L1/L2 are trusted to set checkin-needed on "self-r+'ed" patches. I've heard others state that they will vouch for / approve L1 access for anyone who asks, if people are checking in L1 patches with a self r+ that is scary and needs to stop.

My suggestion for solving this problem is have authors acquire level 3 if they are trusted as such. If that's not satisfactory you might need to start a larger discussion around trust and the level system but that is outside of MozReview's scope atm.

Comment 5

[Jonathan Almeida [:jonalmeida]]

Has this been fixed? Looking at bug 1208167, I received an r+ (with nits) that I fixed. After pushing for review, I retained the r+.

Although, note that I do have two reviewers set for this patch so that might be the reason..

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

AFAIK it only takes one reviewer to get r+ (which in itself is another UX issue. E.g. I might need a DOM review in addition to a peer review).

Comment 7

[Mark Côté [:mcote]]

r+ carryforward for non-L3 authors is still an open question.  I believe dveditz is looking into a general policy that would also concern checkin-needed.

Comment 8

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.