1469380 - Don't include group reviewers in commit-message reviewer string

Status: RESOLVED WONTFIX

(Conduit :: Lando, enhancement)

Description

[Mark Côté [:mcote]]

With some teams using a review-queue approach by specifying a project as a reviewer, we want to avoid having those groups end up in the reviewer string in the commit-message summary (e.g. https://hg.mozilla.org/hgcustom/version-control-tools/rev/65e60e188e4d).

Admittedly, a better workflow is to have the reviewer remove the group and add themselves as a reviewer, which not only solves the problem but also clearly indicates that that person has taken ownership of the review.  However even in that case mistakes might happen, so we may as well filter out the project, which has no meaningful presence in the reviewer string.

Comment 1

[Steven MacLeod [:smacleod]]

(In reply to Mark Côté [:mcote] from comment #0)
> With some teams using a review-queue approach by specifying a project as a
> reviewer, we want to avoid having those groups end up in the reviewer string
> in the commit-message summary (e.g.
> https://hg.mozilla.org/hgcustom/version-control-tools/rev/65e60e188e4d).

That commit was explicitly accepted as the group. When a reviewer is part of a project that is flagged for review they are given two checkboxes when accepting the review, allowing them to accept as themselves, the project, or both. I'd argue the current Lando behavior is correct, considering this.

> Admittedly, a better workflow is to have the reviewer remove the group and
> add themselves as a reviewer, which not only solves the problem but also
> clearly indicates that that person has taken ownership of the review. 
> However even in that case mistakes might happen, so we may as well filter
> out the project, which has no meaningful presence in the reviewer string.

I disagree with both that we should filter it, and that its presence is not meaningful.

Comment 2

[Mark Côté [:mcote]]

(In reply to Steven MacLeod [:smacleod] from comment #1)
> (In reply to Mark Côté [:mcote] from comment #0)
> > With some teams using a review-queue approach by specifying a project as a
> > reviewer, we want to avoid having those groups end up in the reviewer string
> > in the commit-message summary (e.g.
> > https://hg.mozilla.org/hgcustom/version-control-tools/rev/65e60e188e4d).
> 
> That commit was explicitly accepted as the group. When a reviewer is part of
> a project that is flagged for review they are given two checkboxes when
> accepting the review, allowing them to accept as themselves, the project, or
> both. I'd argue the current Lando behavior is correct, considering this.

Ah, thanks for clarifying.  I still don't think it's particularly meaningful in the commit message, but if a user selects it, then so be it.

Comment 3

[Tom Prince [:tomprince]]

(In reply to Mark Côté [:mcote] from comment #2)
> > That commit was explicitly accepted as the group. When a reviewer is part of
> > a project that is flagged for review they are given two checkboxes when
> > accepting the review, allowing them to accept as themselves, the project, or
> > both. I'd argue the current Lando behavior is correct, considering this.
> 
> Ah, thanks for clarifying.  I still don't think it's particularly meaningful
> in the commit message, but if a user selects it, then so be it.

I'm not sure that I agree with the logic there. Certainly the reviewer can accept it as themselves, or as the project. But, at least my assumption of the intent behind that the review is satisfying the requirement to be reviewed by the given team (for example [1]), but not that they should not be credited as the reviewer. (They could also accept it as themselves and also remove the review request from the project, which is what was done in mozreview for the shared build review queue. But that was a workaround for the lack of teams in mozreview).

I don't think that this should be closed as WONTFIX, without a wider discussion whether reviews should be credited just to teams, rather than individuals.

[1] https://phabricator.services.mozilla.com/project/view/20/

Comment 4

[Steven MacLeod [:smacleod]]

(In reply to Tom Prince [:tomprince] from comment #3)
> I don't think that this should be closed as WONTFIX, without a wider
> discussion whether reviews should be credited just to teams, rather than
> individuals.

It wasn't credited to just a team, the commit message includes both.

Comment 6

[Mark Côté [:mcote]]

We're going to come up with guidelines for reviewer groups, which Phabricator has nice support for, but which is also something we have to be careful with given the fact that we use projects for security groups as well.  It is likely that the recommendation there will be to have reviewers *not* review as groups.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

If we are not going to strip out group reviewer from the commit message, we should probably make the Accept as group reviewer checkbox not ticked by default.