cmd/gopherbot: fix reviewer assignment for CLs imported from PRs
golang/go#30265 causes Gerrit to add the authors of PRs as reviewers on
the imported CLs. Then, when Gopherbot assigns reviewers to the PR, it
seems a human reviewer already listed, meaning that a lot of CLs never
get assigned reviewers and are then never replied to.
As a work-around for these issues, require two reviewers on all PRs.
Fixes golang/go#31658
Change-Id: I7b3f62194450cba61493d8c4c0ad14320443e641
Reviewed-on: https://go-review.googlesource.com/c/build/+/227977
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go
index 74a79a3..dcdf684 100644
--- a/cmd/gopherbot/gopherbot.go
+++ b/cmd/gopherbot/gopherbot.go
@@ -1973,7 +1973,22 @@
// The gerritChange passed must be used because it’s used as a key to deletedChanges and the ID returned
// by cl.ChangeID() can be associated with multiple changes (cherry-picks, for example).
func (b *gopherbot) humanReviewersOnChange(ctx context.Context, change gerritChange, cl *maintner.GerritCL) bool {
- if found := humanReviewersInMetas(cl.Metas); found {
+ const (
+ gobotID = 5976
+ gerritbotID = 12446
+ )
+ // The CL's owner will be GerritBot if it is imported from a PR.
+ // In that case, if the CL's author has a Gerrit account, they will be
+ // added as a reviewer (golang.org/issue/30265). Otherwise, no reviewers
+ // will be added. Work around this by requiring 2 human reviewers on PRs.
+ ownerID := int64(cl.OwnerID())
+ isPR := ownerID == gerritbotID
+ minHumans := 1
+ if isPR {
+ minHumans = 2
+ }
+
+ if found := humanReviewersInMetas(cl.Metas, minHumans); found {
return true
}
@@ -1985,27 +2000,27 @@
log.Printf("Could not list reviewers on change %q: %v", change.ID(), err)
return true
}
-
- const (
- gobotID = 5976
- gerritbotID = 12446
- )
+ var count int
for _, r := range reviewers {
- if r.NumericID != gobotID && r.NumericID != gerritbotID {
- return true
+ if r.NumericID != gobotID && r.NumericID != gerritbotID && r.NumericID != ownerID {
+ count++
+ if count == minHumans {
+ return true
+ }
}
}
return false
}
-func humanReviewersInMetas(metas []*maintner.GerritMeta) bool {
+// humanReviewersInMetas reports whether there are at least minHumans human
+// reviewers in the given metas.
+func humanReviewersInMetas(metas []*maintner.GerritMeta, minHumans int) bool {
// Emails as they appear in maintner (<numeric ID>@<instance ID>)
var (
gobotEmail = "5976@62eb7196-b449-3ce5-99f1-c037f21e1705"
gerritbotEmail = "12446@62eb7196-b449-3ce5-99f1-c037f21e1705"
-
- hasHuman bool
)
+ var count int
for _, m := range metas {
if !strings.Contains(m.Commit.Msg, "Reviewer:") && !strings.Contains(m.Commit.Msg, "CC:") {
continue
@@ -2017,17 +2032,19 @@
}
if !strings.Contains(ln, gobotEmail) && !strings.Contains(ln, gerritbotEmail) {
// A human is already on the change.
- hasHuman = true
- return errStopIteration
+ count++
+ if count == minHumans {
+ return errStopIteration
+ }
}
return nil
})
if err != nil && err != errStopIteration {
log.Printf("humanReviewersInMetas: got unexpected error from foreach.LineStr: %v", err)
- return hasHuman
+ return count >= minHumans
}
}
- return hasHuman
+ return count >= minHumans
}
func getCodeOwners(ctx context.Context, paths []string) ([]*owners.Entry, error) {
diff --git a/cmd/gopherbot/gopherbot_test.go b/cmd/gopherbot/gopherbot_test.go
index 642407c..5af4a08 100644
--- a/cmd/gopherbot/gopherbot_test.go
+++ b/cmd/gopherbot/gopherbot_test.go
@@ -356,33 +356,54 @@
testCases := []struct {
commitMsg string
hasHuman bool
+ atLeast int
}{
{`Patch-set: 6
Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
+ 1,
},
{`Patch-set: 6
CC: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
+ 1,
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
false,
+ 1,
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
CC: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
+ 1,
},
{`Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
true,
+ 1,
+ },
+ {`Patch-set: 6
+Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
+Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
+ `,
+ false,
+ 2,
+ },
+ {`Patch-set: 6
+Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
+Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
+Reviewer: Rebecca Stambler <16140@62eb7196-b449-3ce5-99f1-c037f21e1705>
+ `,
+ true,
+ 2,
},
}
@@ -390,7 +411,7 @@
metas := []*maintner.GerritMeta{
{Commit: &maintner.GitCommit{Msg: tc.commitMsg}},
}
- if got, want := humanReviewersInMetas(metas), tc.hasHuman; got != want {
+ if got, want := humanReviewersInMetas(metas, tc.atLeast), tc.hasHuman; got != want {
t.Errorf("Unexpected result for meta commit message: got %v; want %v for\n%s", got, want, tc.commitMsg)
}
}
