Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gerrit: add new Trust+2 label requirement for code reviews #40699

Closed
rsc opened this issue Aug 11, 2020 · 11 comments
Closed

gerrit: add new Trust+2 label requirement for code reviews #40699

rsc opened this issue Aug 11, 2020 · 11 comments
Assignees
Labels
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Aug 11, 2020

As Go grows, we're reviewing code review practices and what is possible for a single rogue approver (or hacked approver's machine) to do unilaterally.

The general rule we've tried to follow in Go is the same one used inside Google: two people are required to make a change to the code base. For Go, that's the author and the separate code reviewer who approves (Code-Review +2's) the change. As part of Go maturing, we've taken a closer look at our practices around this rule and found two violations in today's practices:

  1. There's no check that the author is someone known to us, as opposed to a sock-puppet or other hijacked account.
  2. Nothing but social pressure stops an author who is an approver from approving and submitting their own changes.

In order to follow the two trusted approvers rule more closely, we intend to add a new “Trust” label, separate from the “Code-Review” label. Everyone who can approve (Code-Review+2) a change will be able to Trust+1 the change as well, and Gerrit will require two separate Trust+1 votes (adding to Trust+2, unlike Code-Review math) for a submit.

It is an explicit goal to minimize the burden of this new requirement. To that end, we will change the git codereview mail command to automatically add the Trust+1 vote during the mail operation when the person mailing the change is themselves an approver. Then the code reviewer simply has to remember to Trust+1 the change along with the Code-Review+2. (As far as we understand, Gerrit's Prolog rules do not allow us to make Code-Review+2 trigger Trust+1 automatically, so the extra click during review is unavoidable.)

  • For changes where both the author and reviewer are approvers (the vast majority of our changes), the author provides a Trust+1, the reviewer provides a Trust+1, and the change can be submitted in the same number of steps as before (one extra click during review).
  • For changes where the author is not an approver, two different approvers will need to contribute Trust+1 votes. This is an intentional change from current practices, as explained above.
  • For changes where the author and reviewer are the same person, a second reviewer will be needed. This too is an intentional change from current practices, as explained above.

The Trust+2 requirement will not apply to the proposal and blog repos, which hold documents, not production code. Self-+2'ing changes there will continue to be allowed for docs and pre-reviewed blog posts.

@rsc rsc added the NeedsFix label Aug 11, 2020
@rsc rsc added this to the Unreleased milestone Aug 11, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 11, 2020

Do we need a separate Trust label? Can we just require two Code-Review+2 votes and make git codereview mail automatically set that?

https://gerrit-review.googlesource.com/Documentation/prolog-change-facts.html mentions the commit_label/2 fact, which seems like it can match on individual per-user votes. It seems like something like this should work:

commit_label(label('Code-Review', 2), user(A)),
commit_label(label('Code-Review', 2), user(B)),
A \= B.

(Caveat: I haven't used Prolog in a long time.)

@ALTree
Copy link
Member

@ALTree ALTree commented Aug 11, 2020

@mdempsky IMO two different labels are better because they'll allow for example me to say

Trust+1, I've looked at this change and it seems okay and not harmful, but wait for the package-owner code review for a technical decision on this

With only a single label Codereview+2 you wouldn't be able to distinguish between a technical review and decision (answering: "Is this correct? Is this a good idea?") and a trust rubber-stamp ("This looks like it's not harmful").

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 12, 2020

Reading through Gerrit's Prolog Submit Rules Cookbook, it seems like it should be fine to make the submit rule "there must be a Code-Review+2 or Trust+1 vote from at least two different users". Then no one ever needs to click both Code-Review+2 and Trust+1.

For example, I expect this should work:

trust(U) :- gerrit:commit_label(label('Code-Review', 2), U).
trust(U) :- gerrit:commit_label(label('Trust', 1), U).

trust2(ok(U1), ok(U2)) :- trust(U1), trust(U2), U1 \= U2.
trust2(ok(U1), need(_)) :- trust(U1).
trust2(need(_), need(_)).

trust_rule(label('Trust1', U1), label('Trust2', U2)) :- trust2(U1, U2).

submit_rule(submit(V, CR, T1, T2)) :-
    %% Default submit_rule logic: require Code-Review+2 and Verified+1,
    %% and block if Code-Review-2 or Verified-1.
    gerrit:max_with_block(-2, 2, 'Code-Review', CR),
    gerrit:max_with_block(-1, 1, 'Verified', V),

    %% The new rule: require `Code-Review+2` or `Trust+1` from two separate users.
    trust_rule(T1, T2).
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 13, 2020

Just a reminder that there are people with multiple accounts in the approver set and we'll need to merge them or drop one of them. (I assume they mostly use one of them, so I don't expect this to be a problem.)

@rasky
Copy link
Member

@rasky rasky commented Aug 17, 2020

Who will be allowed to add a Trust+1? Is that the same ACL of CodeReview+2, or is it a different one?

What would be the suggested policy for reviewers about when to add Trust+1, and how is that different from the current practice of adding CodeReview+1 when you have reviewed the code, think the code is correct but you don't feel knowledgable enough to approve it?

@ALTree
Copy link
Member

@ALTree ALTree commented Aug 18, 2020

@rasky

Who will be allowed to add a Trust+1?

From the first post:

Everyone who can approve (Code-Review+2) a change will be able to Trust+1 the change as well

@rsc
Copy link
Contributor Author

@rsc rsc commented Aug 27, 2020

@mdempsky, sorry for not seeing your question earlier.
No, we do not want to overload CodeReview+2.
As a matter of policy, I don't want authors appearing to +2
their own changes to become a standard practice.
It's a bad look: authors don't review their own code.
Two distinct labels separates the concerns much more clearly.

@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 11, 2020

Just a reminder that there are people with multiple accounts in the approver set and we'll need to merge them or drop one of them.

Let's fix that. Everyone in the approver set should have one account (in the set).

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 11, 2020

Change https://golang.org/cl/254421 mentions this issue: git-codereview: add mail -trust to set Trust+1 vote

gopherbot pushed a commit to golang/review that referenced this issue Sep 11, 2020
The expectation is that approvers will change their .gitconfig
to define mail as codereview mail -trust.

For golang/go#40699.

Change-Id: I2a1040bf3f1c7248e9c361e1f2a83c17870b1df5
Reviewed-on: https://go-review.googlesource.com/c/review/+/254421
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc
Copy link
Contributor Author

@rsc rsc commented Sep 14, 2020

I've updated the Gerrit server config. Thanks very much for the Prolog tips @mdempsky.
In case anyone else needs it, this is the All-Projects rules.pl we are using, with the noted custom rules.pl for blog and proposal:

% submit_rule in the actual project (go, scratch, etc.)
% generates a list of labels and whether they are ok, needed, or can be ignored.
% Most repos don't define a specific submit_rule; the server uses gerrit:default_submit.
%
% submit_filter, defined below, is applied to the output of submit_rule to adjust it.
% The adjustments are:
%
%  * do_not_review_filter: add a “rejected by Do-Not-Review label”
%    if the commit message says DO NOT REVIEW.
%
%  * do_not_submit_filter: add a “rejected by Do-Not-Submit label”
%    if the commit message says DO NOT SUBMIT.
%
%  * trust_filter: make sure that two different people have set
%    either Code-Review+2 or Trust+1 on the commit.
%
%  * self_review_filter: make sure that Code-Review+2
%    is from someone other than the author of the commit.
%
% Note that if you make any mistake in this file that causes
% a Prolog execution exception or makes submit_filter not succeed,
% the usual signal you get in Gerrit is that labels disappear from the UI for all CLs.
% Sometimes you get an internal error popup instead (syntax errors do this, for example).

submit_filter(In, Out) :-
	Gobot = user(5976),
	In =.. [submit | A],
	do_not_review_filter(Gobot, A, B),
	do_not_submit_filter(Gobot, B, C),
	trust_filter(Gobot, C, D),
	self_review_filter(Gobot, D, E),
	Out =.. [submit | E].

% We don't send email if the commit message says DO NOT REVIEW,
% so don't allow submit either.
% Case insensitive because the mail filter rules are.
do_not_review_filter(Gobot, In, Out) :-
	gerrit:commit_message_matches('[Dd][Oo][ \t\r\n]+[Nn][Oo][Tt][ \t\r\n]+[Rr][Ee][Vv][Ii][Ee][Ww]'),
	!,
	gerrit:commit_author(Id),
	Error = label('Do-Not-Review', reject(Id)),
	Out = [Error | In].

do_not_review_filter(Gobot, In, Out) :-
	Out = In.

% Don't allow submit of DO NOT SUBMIT commit message either.
% Case insensitive because DO NOT REVIEW is.
do_not_submit_filter(Gobot, In, Out) :-
	gerrit:commit_message_matches('[Dd][Oo][ \t\r\n]+[Nn][Oo][Tt][ \t\r\n]+[Ss][Uu][Bb][Mm][Ii][Tt]'),
	!,
	gerrit:commit_author(Id),
	Error = label('Do-Not-Submit', reject(Id)),
	Out = [Error | In].

do_not_submit_filter(Gobot, In, Out) :-
	Out = In.

% Check for two trusted author/reviewers, unless Self-Review-OK label set by project.
trust_filter(Gobot, In, Out) :- has_label('Self-Review-OK', In), !, Out = In.
trust_filter(Gobot, In, Out) :- trust2, !, Out = [label('Trust-2', ok(Gobot)) | In].
trust_filter(Gobot, In, Out) :- Out = [label('Trust-2', reject(Gobot)) | In].

trust2 :- trust(U1), trust(U2), U1 \= U2.

trust(U) :- gerrit:commit_label(label('Code-Review', 2), U).
trust(U) :- gerrit:commit_label(label('Trust', 1), U).

% Reject self-code-review, unless Self-Review-OK label set by project.
%
% NOTE: Projects (= repos) that allow self-code-review and that do not require Trust+2
% (for example, the blog and proposal projects), can use this in rules.pl to set Self-Review-OK:
%
%	submit_rule(S) :-
%		Gobot = user(5976),
%		gerrit:default_submit(R),
%		R =.. [submit | A],
%		self_review_ok(Gobot, A, B),
%		S =.. [submit | B].
%
%	self_review_ok(Gobot, In, Out) :- Out = [label('Self-Review-OK', ok(Gobot)) | In].
%
self_review_filter(Gobot, In, Out) :- has_label('Self-Review-OK', In), !, Out = In.
self_review_filter(Gobot, In, Out) :- self_review, !, Out = [label('Self-Review', reject(Gobot)) | In].
self_review_filter(Gobot, In, Out) :- Out = In.

self_review :-
	gerrit:change_owner(Owner),
	gerrit:commit_label(label('Code-Review', 2), Owner).

% has_label checks whether the list contains the label L.
has_label(L, [label(L, _) | T]).
has_label(L, [_ | T]) :- has_label(L, T).
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 5, 2020

Change https://golang.org/cl/259737 mentions this issue: doc: mention Trust+1 in contribution guide

gopherbot pushed a commit that referenced this issue Oct 28, 2020
For #40699

Change-Id: If753a073488880433ae3319dcf2a2dfaa887fd0e
Reviewed-on: https://go-review.googlesource.com/c/go/+/259737
Trust: Ian Lance Taylor <iant@golang.org>
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.