cmd/gopherbot: drop non-Gerrit owners for review assignment
Owners may include users that do not have an associated Gerrit account,
notably GitHub Teams. When determining reviewers to add a CL, drop all
these users from the potential reviewer set. Otherwise Gerrit chokes on
attempting to set reviewer to "".
Dropping these users from owners may result in an empty primary reviewer
set (e.g., on packages where a GitHub team is the only primary owner).
In this case, upgrade the secondary owners to primary so they end up as
reviewers rather than just CC'd. If both lists are empty, a 'no-owners'
tag is added to the CL by assignReviewersToCLs.
For golang/go#50723.
For golang/go#28320.
Change-Id: I99ac5426e080bf32c46817c09debeac507282e01
Reviewed-on: https://go-review.googlesource.com/c/build/+/386754
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/cmd/gopherbot/gopherbot.go b/cmd/gopherbot/gopherbot.go
index 3ac8d7f..9a6b3fd 100644
--- a/cmd/gopherbot/gopherbot.go
+++ b/cmd/gopherbot/gopherbot.go
@@ -2044,6 +2044,9 @@
return nil
}
+ // Remove owners that can't be reviewers.
+ entries = filterGerritOwners(entries)
+
authorEmail := cl.Commit.Author.Email()
merged := mergeOwnersEntries(entries, authorEmail)
if len(merged.Primary) == 0 && len(merged.Secondary) == 0 {
@@ -2506,6 +2509,35 @@
return &result
}
+// filterGerritOwners removes all primary and secondary owners from entries
+// that are missing GerritEmail, and thus cannot be Gerrit reviewers (e.g.,
+// GitHub Teams).
+//
+// If an Entry's primary reviewers is empty after this process, the secondary
+// owners are upgraded to primary.
+func filterGerritOwners(entries []*owners.Entry) []*owners.Entry {
+ result := make([]*owners.Entry, 0, len(entries))
+ for _, e := range entries {
+ var clean owners.Entry
+ for _, owner := range e.Primary {
+ if owner.GerritEmail != "" {
+ clean.Primary = append(clean.Primary, owner)
+ }
+ }
+ for _, owner := range e.Secondary {
+ if owner.GerritEmail != "" {
+ clean.Secondary = append(clean.Secondary, owner)
+ }
+ }
+ if len(clean.Primary) == 0 {
+ clean.Primary = clean.Secondary
+ clean.Secondary = nil
+ }
+ result = append(result, &clean)
+ }
+ return result
+}
+
func blockqoute(s string) string {
s = strings.TrimSpace(s)
s = "> " + s
diff --git a/cmd/gopherbot/gopherbot_test.go b/cmd/gopherbot/gopherbot_test.go
index cc08011..9493e9d 100644
--- a/cmd/gopherbot/gopherbot_test.go
+++ b/cmd/gopherbot/gopherbot_test.go
@@ -518,3 +518,105 @@
}
}
}
+
+func TestFilterGerritOwners(t *testing.T) {
+ var (
+ andybons = owners.Owner{GitHubUsername: "andybons", GerritEmail: "andybons@golang.org"}
+ bradfitz = owners.Owner{GitHubUsername: "bradfitz", GerritEmail: "bradfitz@golang.org"}
+ toolsTeam = owners.Owner{GitHubUsername: "golang/tools-team"}
+ )
+ testCases := []struct {
+ name string
+ entries []*owners.Entry
+ want []*owners.Entry
+ }{
+ {
+ name: "no entries",
+ entries: nil,
+ want: []*owners.Entry{},
+ },
+ {
+ name: "all valid",
+ entries: []*owners.Entry{
+ {Primary: []owners.Owner{andybons}},
+ {Primary: []owners.Owner{bradfitz}},
+ },
+ want: []*owners.Entry{
+ {Primary: []owners.Owner{andybons}},
+ {Primary: []owners.Owner{bradfitz}},
+ },
+ },
+ {
+ name: "drop primary",
+ entries: []*owners.Entry{
+ {Primary: []owners.Owner{andybons, toolsTeam}},
+ {Primary: []owners.Owner{toolsTeam, bradfitz}},
+ },
+ want: []*owners.Entry{
+ {Primary: []owners.Owner{andybons}},
+ {Primary: []owners.Owner{bradfitz}},
+ },
+ },
+ {
+ name: "drop secondary",
+ entries: []*owners.Entry{
+ {
+ Primary: []owners.Owner{andybons},
+ Secondary: []owners.Owner{bradfitz, toolsTeam},
+ },
+ {
+ Primary: []owners.Owner{bradfitz},
+ Secondary: []owners.Owner{toolsTeam, andybons},
+ },
+ },
+ want: []*owners.Entry{
+ {
+ Primary: []owners.Owner{andybons},
+ Secondary: []owners.Owner{bradfitz},
+ },
+ {
+ Primary: []owners.Owner{bradfitz},
+ Secondary: []owners.Owner{andybons},
+ },
+ },
+ },
+ {
+ name: "upgrade secondary",
+ entries: []*owners.Entry{
+ {
+ Primary: []owners.Owner{toolsTeam},
+ Secondary: []owners.Owner{bradfitz},
+ },
+ },
+ want: []*owners.Entry{
+ {
+ Primary: []owners.Owner{bradfitz},
+ },
+ },
+ },
+ {
+ name: "no primary",
+ entries: []*owners.Entry{
+ {
+ Secondary: []owners.Owner{bradfitz},
+ },
+ },
+ want: []*owners.Entry{
+ {
+ Primary: []owners.Owner{bradfitz},
+ },
+ },
+ },
+ }
+ cmpFn := func(a, b owners.Owner) bool {
+ return a.GitHubUsername < b.GitHubUsername
+ }
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ got := filterGerritOwners(tc.entries)
+ if diff := cmp.Diff(got, tc.want, cmpopts.SortSlices(cmpFn)); diff != "" {
+ t.Errorf("final entry results differ: (-got, +want)\n%s", diff)
+ }
+ })
+ }
+}
