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
Comments
Do we need a separate https://gerrit-review.googlesource.com/Documentation/prolog-change-facts.html mentions the
(Caveat: I haven't used Prolog in a long time.) |
@mdempsky IMO two different labels are better because they'll allow for example me to say
With only a single label |
Reading through Gerrit's Prolog Submit Rules Cookbook, it seems like it should be fine to make the submit rule "there must be a For example, I expect this should work:
|
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.) |
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? |
From the first post:
|
@mdempsky, sorry for not seeing your question earlier. |
Let's fix that. Everyone in the approver set should have one account (in the set). |
Change https://golang.org/cl/254421 mentions this issue: |
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>
I've updated the Gerrit server config. Thanks very much for the Prolog tips @mdempsky.
|
Change https://golang.org/cl/259737 mentions this issue: |
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>
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:
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.)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.
The text was updated successfully, but these errors were encountered: