cmd/gopherbot: omit owner from assign reviewers
Assign reviewers correctly omits the owner from the list of gerrit
reviewers, but not from the list of metadata on the CL. When adding
a "Run-TryBot +1" vote to a CL, the owner is listed as a Reviewer in the
metadata.
This change updates both cases to use the same filter, simplifying the
humanReviewersOnChange to check for bots and owner the same way for
metadata and reviewers.
Fixes golang/go#53077
Change-Id: Ia3d4939f6fb8d81cfb371683025b5e3ce0282fd7
Reviewed-on: https://go-review.googlesource.com/c/build/+/417479
Run-TryBot: Jenny Rakoczy <jenny@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Jenny Rakoczy <jenny@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go
index 2bf3eb7..2ef8392 100644
--- a/cmd/gopherbot/gopherbot.go
+++ b/cmd/gopherbot/gopherbot.go
@@ -75,6 +75,12 @@
gopherbotGitHubID = 8566911
)
+const (
+ gobotGerritID = "5976"
+ gerritbotGerritID = "12446"
+ kokoroGerritID = "37747"
+)
+
// GitHub Label IDs for the golang/go repo.
const (
needsDecisionID = 373401956
@@ -2322,24 +2328,29 @@
// 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) ([]string, bool) {
- const (
- gobotID = 5976
- gerritbotID = 12446
- kokoroID = 37747
- )
// 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
+ ownerID := strconv.Itoa(cl.OwnerID())
+ isPR := ownerID == gerritbotGerritID
minHumans := 1
if isPR {
minHumans = 2
}
+ reject := []string{gobotGerritID, gerritbotGerritID, kokoroGerritID, ownerID}
+ ownerOrRobot := func(gerritID string) bool {
+ for _, r := range reject {
+ if gerritID == r {
+ return true
+ }
+ }
+ return false
+ }
- if reviewers, found := humanReviewersInMetas(cl.Metas, minHumans); found {
- return reviewers, true
+ ids := deleteStrings(reviewersInMetas(cl.Metas), ownerOrRobot)
+ if len(ids) >= minHumans {
+ return ids, true
}
reviewers, err := b.gerrit.ListReviewers(ctx, change.ID())
@@ -2350,22 +2361,29 @@
log.Printf("Could not list reviewers on change %q: %v", change.ID(), err)
return nil, true
}
- var ids []string
+ ids = []string{}
for _, r := range reviewers {
- switch id := r.NumericID; {
- case id == gobotID, id == gerritbotID, id == kokoroID,
- hasServiceUserTag(r.AccountInfo):
- // Skip bots.
- continue
- case id == ownerID:
- // Skip owner.
+ id := strconv.FormatInt(r.NumericID, 10)
+ if hasServiceUserTag(r.AccountInfo) || ownerOrRobot(id) {
+ // Skip bots and owner.
continue
}
- ids = append(ids, strconv.FormatInt(r.NumericID, 10))
+ ids = append(ids, id)
}
return ids, len(ids) >= minHumans
}
+func deleteStrings(s []string, reject func(val string) bool) []string {
+ var filtered []string
+ for _, val := range s {
+ if reject(val) {
+ continue
+ }
+ filtered = append(filtered, val)
+ }
+ return filtered
+}
+
// hasServiceUserTag reports whether the account has a SERVICE_USER tag.
func hasServiceUserTag(a gerrit.AccountInfo) bool {
for _, t := range a.Tags {
@@ -2579,17 +2597,8 @@
const gerritInstanceID = "@62eb7196-b449-3ce5-99f1-c037f21e1705"
-// humanReviewersInMetas reports whether there are at least minHumans human
-// reviewers in the given metas. It also returns the Gerrit IDs of all of the
-// human reviewers.
-func humanReviewersInMetas(metas []*maintner.GerritMeta, minHumans int) ([]string, bool) {
- // Emails as they appear in maintner (<numeric ID>@<instance ID>)
- var (
- gobotEmail = "5976" + gerritInstanceID
- gerritbotEmail = "12446" + gerritInstanceID
- kokoroEmail = "37747" + gerritInstanceID
- )
- var count int
+// reviewersInMetas returns the Gerrit IDs of any reviewers in the metadata.
+func reviewersInMetas(metas []*maintner.GerritMeta) []string {
var ids []string
for _, m := range metas {
if !strings.Contains(m.Commit.Msg, "Reviewer:") && !strings.Contains(m.Commit.Msg, "CC:") {
@@ -2600,32 +2609,27 @@
if !strings.HasPrefix(ln, "Reviewer:") && !strings.HasPrefix(ln, "CC:") {
return nil
}
- if !strings.Contains(ln, gobotEmail) && !strings.Contains(ln, gerritbotEmail) &&
- !strings.Contains(ln, kokoroEmail) {
- match := reviewerRe.FindStringSubmatch(ln)
- if match == nil {
- return nil
+ match := reviewerRe.FindStringSubmatch(ln)
+ if match == nil {
+ return nil
+ }
+ // Extract the human's Gerrit ID.
+ for i, name := range reviewerRe.SubexpNames() {
+ if name != "id" {
+ continue
}
- // A human is already on the change.
- count++
- // Extract the human's Gerrit ID.
- for i, name := range reviewerRe.SubexpNames() {
- if name != "id" {
- continue
- }
- if i < 0 || i > len(match) {
- continue
- }
- ids = append(ids, match[i])
+ if i < 0 || i > len(match) {
+ continue
}
+ ids = append(ids, match[i])
}
return nil
})
if err != nil {
- log.Printf("humanReviewersInMetas: got unexpected error from foreach.LineStr: %v", err)
+ log.Printf("reviewersInMetas: got unexpected error from foreach.LineStr: %v", err)
}
}
- return ids, count >= minHumans
+ return ids
}
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 712ff22..39577d6 100644
--- a/cmd/gopherbot/gopherbot_test.go
+++ b/cmd/gopherbot/gopherbot_test.go
@@ -355,85 +355,74 @@
func TestHumanReviewersInMetas(t *testing.T) {
testCases := []struct {
+ desc string
commitMsg string
hasHuman bool
- atLeast int
wantIDs []string
}{
- {`Patch-set: 6
+ {
+ desc: "one human reviewer",
+ commitMsg: `Patch-set: 6
Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
- true,
- 1,
- []string{"22285"},
+ hasHuman: true,
+ wantIDs: []string{"22285"},
},
- {`Patch-set: 6
+ {
+ desc: "one human CC",
+ commitMsg: `Patch-set: 6
CC: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
- true,
- 1,
- []string{"22285"},
+ hasHuman: true,
+ wantIDs: []string{"22285"},
},
- {`Patch-set: 6
+ {
+ desc: "gobot reviewer",
+ commitMsg: `Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
- false,
- 1,
- []string{},
+ wantIDs: []string{"5976"},
},
- {`Patch-set: 6
+ {
+ desc: "gobot reviewer and human CC",
+ commitMsg: `Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
CC: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
- true,
- 1,
- []string{"22285"},
+ hasHuman: true,
+ wantIDs: []string{"5976", "22285"},
},
- {`Patch-set: 6
+ {
+ desc: "gobot reviewer and human reviewer",
+ commitMsg: `Patch-set: 6
Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
`,
- true,
- 1,
- []string{"22285"},
+ hasHuman: true,
+ wantIDs: []string{"5976", "22285"},
},
- {`Patch-set: 6
-Reviewer: Gobot Gobot <5976@62eb7196-b449-3ce5-99f1-c037f21e1705>
-Reviewer: Andrew Bonventre <22285@62eb7196-b449-3ce5-99f1-c037f21e1705>
- `,
- false,
- 2,
- []string{"22285"},
- },
- {`Patch-set: 6
+ {
+ desc: "gobot reviewer and two human reviewers",
+ commitMsg: `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,
- []string{"22285", "16140"},
+ hasHuman: true,
+ wantIDs: []string{"5976", "22285", "16140"},
},
}
for _, tc := range testCases {
- metas := []*maintner.GerritMeta{
- {Commit: &maintner.GitCommit{Msg: tc.commitMsg}},
- }
- ids, got := humanReviewersInMetas(metas, tc.atLeast)
- if want := tc.hasHuman; got != want {
- t.Errorf("Unexpected result for meta commit message: got %v; want %v for\n%s", got, want, tc.commitMsg)
- continue
- }
- if len(ids) != len(tc.wantIDs) {
- t.Errorf("Unexpected result for meta commit message: got %v reviewer IDs, want %v", len(ids), len(tc.wantIDs))
- continue
- }
- for i, id := range tc.wantIDs {
- if id != ids[i] {
- t.Errorf("Unexpected ID at %d: got %v, want %v", i, ids[i], id)
+ t.Run(tc.desc, func(t *testing.T) {
+ metas := []*maintner.GerritMeta{
+ {Commit: &maintner.GitCommit{Msg: tc.commitMsg}},
}
- }
+ ids := reviewersInMetas(metas)
+ if diff := cmp.Diff(tc.wantIDs, ids); diff != "" {
+ t.Fatalf("reviewersInMetas() mismatch (-want +got):\n%s", diff)
+ }
+ })
}
}
