From 48d7aae3b75f822514c16c205f3750365b87d095 Mon Sep 17 00:00:00 2001 From: Grant Rodgers Date: Thu, 12 Apr 2018 16:23:51 -0700 Subject: [PATCH] Suggest approvers if reviewers aren't available If there aren't enough qualified reviewers available (as determined by the reviewer_count option), then fall back to using approvers as reviewers. This is most useful for projects that don't have a reviewer role. Those projects may now use OWNERS files containing only approvers lists. This is now the default behavior, so approvers which are not in any reviewer lists may now get review requests when they didn't before. To return to the previous behavior of only considering reviewers, set the exclude_approvers option to true. --- prow/plugins/blunderbuss/blunderbuss.go | 54 ++++- prow/plugins/blunderbuss/blunderbuss_test.go | 238 ++++++++++++++++--- prow/plugins/plugins.go | 5 + 3 files changed, 260 insertions(+), 37 deletions(-) diff --git a/prow/plugins/blunderbuss/blunderbuss.go b/prow/plugins/blunderbuss/blunderbuss.go index e8a5978b8a1d..040c80387799 100644 --- a/prow/plugins/blunderbuss/blunderbuss.go +++ b/prow/plugins/blunderbuss/blunderbuss.go @@ -60,12 +60,35 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin nil } -type ownersClient interface { +type reviewersClient interface { FindReviewersOwnersForFile(path string) string Reviewers(path string) sets.String LeafReviewers(path string) sets.String } +type ownersClient interface { + reviewersClient + FindApproverOwnersForFile(path string) string + Approvers(path string) sets.String + LeafApprovers(path string) sets.String +} + +type fallbackReviewersClient struct { + ownersClient +} + +func (foc fallbackReviewersClient) FindReviewersOwnersForFile(path string) string { + return foc.ownersClient.FindApproverOwnersForFile(path) +} + +func (foc fallbackReviewersClient) Reviewers(path string) sets.String { + return foc.ownersClient.Approvers(path) +} + +func (foc fallbackReviewersClient) LeafReviewers(path string) sets.String { + return foc.ownersClient.LeafApprovers(path) +} + type githubClient interface { RequestReview(org, repo string, number int, logins []string) error GetPullRequestChanges(org, repo string, number int) ([]github.PullRequestChange, error) @@ -87,11 +110,12 @@ func handlePullRequest(pc plugins.PluginClient, pre github.PullRequestEvent) err pc.PluginConfig.Blunderbuss.ReviewerCount, pc.PluginConfig.Blunderbuss.FileWeightCount, pc.PluginConfig.Blunderbuss.MaxReviewerCount, + pc.PluginConfig.Blunderbuss.ExcludeApprovers, &pre, ) } -func handle(ghc githubClient, oc ownersClient, log *logrus.Entry, reviewerCount, oldReviewCount *int, maxReviewers int, pre *github.PullRequestEvent) error { +func handle(ghc githubClient, oc ownersClient, log *logrus.Entry, reviewerCount, oldReviewCount *int, maxReviewers int, excludeApprovers bool, pre *github.PullRequestEvent) error { changes, err := ghc.GetPullRequestChanges(pre.Repo.Owner.Login, pre.Repo.Name, pre.Number) if err != nil { return fmt.Errorf("error getting PR changes: %v", err) @@ -107,12 +131,28 @@ func handle(ghc githubClient, oc ownersClient, log *logrus.Entry, reviewerCount, return err } if missing := *reviewerCount - len(reviewers); missing > 0 { - log.Warnf("Not enough reviewers found in OWNERS files for files touched by this PR. %d/%d reviewers found.", len(reviewers), reviewerCount) + log.Warnf("Not enough reviewers found in OWNERS files for files touched by this PR. %d/%d reviewers found.", len(reviewers), *reviewerCount) + if !excludeApprovers { + // Attempt to use approvers as additional reviewers. This must use + // reviewerCount instead of missing because owners can be both reviewers + // and approvers and the search might stop too early if it finds + // duplicates. + frc := fallbackReviewersClient{ownersClient: oc} + approvers, err := getReviewers(frc, pre.PullRequest.User.Login, changes, *reviewerCount) + if err != nil { + return err + } + combinedReviewers := sets.NewString(reviewers...) + combinedReviewers.Insert(approvers...) + log.Warnf("Added %d approvers as reviewers. %d/%d reviewers found.", combinedReviewers.Len()-len(reviewers), combinedReviewers.Len(), *reviewerCount) + reviewers = combinedReviewers.List() + } } } if len(reviewers) > 0 { if maxReviewers > 0 && len(reviewers) > maxReviewers { + log.Warnf("Limiting request of %d reviewers to %d maxReviewers.", len(reviewers), maxReviewers) reviewers = reviewers[:maxReviewers] } log.Infof("Requesting reviews from users %s.", reviewers) @@ -121,20 +161,20 @@ func handle(ghc githubClient, oc ownersClient, log *logrus.Entry, reviewerCount, return nil } -func getReviewers(owners ownersClient, author string, files []github.PullRequestChange, minReviewers int) ([]string, error) { +func getReviewers(rc reviewersClient, author string, files []github.PullRequestChange, minReviewers int) ([]string, error) { authorSet := sets.NewString(author) reviewers := sets.NewString() leafReviewers := sets.NewString() ownersSeen := sets.NewString() // first build 'reviewers' by taking a unique reviewer from each OWNERS file. for _, file := range files { - ownersFile := owners.FindReviewersOwnersForFile(file.Filename) + ownersFile := rc.FindReviewersOwnersForFile(file.Filename) if ownersSeen.Has(ownersFile) { continue } ownersSeen.Insert(ownersFile) - fileUnusedLeafs := owners.LeafReviewers(file.Filename).Difference(reviewers).Difference(authorSet) + fileUnusedLeafs := rc.LeafReviewers(file.Filename).Difference(reviewers).Difference(authorSet) if fileUnusedLeafs.Len() == 0 { continue } @@ -150,7 +190,7 @@ func getReviewers(owners ownersClient, author string, files []github.PullRequest if reviewers.Len() >= minReviewers { break } - fileReviewers := owners.Reviewers(file.Filename).Difference(authorSet) + fileReviewers := rc.Reviewers(file.Filename).Difference(authorSet) for reviewers.Len() < minReviewers && fileReviewers.Len() > 0 { reviewers.Insert(popRandom(fileReviewers)) } diff --git a/prow/plugins/blunderbuss/blunderbuss_test.go b/prow/plugins/blunderbuss/blunderbuss_test.go index c83818d5a9c6..33c9ea1aeb66 100644 --- a/prow/plugins/blunderbuss/blunderbuss_test.go +++ b/prow/plugins/blunderbuss/blunderbuss_test.go @@ -70,10 +70,24 @@ func (c *fakeGithubClient) GetPullRequestChanges(org, repo string, num int) ([]g type fakeOwnersClient struct { owners map[string]string + approvers map[string]sets.String + leafApprovers map[string]sets.String reviewers map[string]sets.String leafReviewers map[string]sets.String } +func (foc *fakeOwnersClient) Approvers(path string) sets.String { + return foc.approvers[path] +} + +func (foc *fakeOwnersClient) LeafApprovers(path string) sets.String { + return foc.leafApprovers[path] +} + +func (foc *fakeOwnersClient) FindApproverOwnersForFile(path string) string { + return foc.owners[path] +} + func (foc *fakeOwnersClient) Reviewers(path string) sets.String { return foc.reviewers[path] } @@ -86,38 +100,34 @@ func (foc *fakeOwnersClient) FindReviewersOwnersForFile(path string) string { return foc.owners[path] } -// TestHandle tests that the handle function requests reviews from the correct number of unique users. -func TestHandle(t *testing.T) { - foc := &fakeOwnersClient{ - owners: map[string]string{ - "a.go": "1", - "b.go": "2", - "bb.go": "3", - "c.go": "4", - - "e.go": "5", - "ee.go": "5", - }, - reviewers: map[string]sets.String{ - "a.go": sets.NewString("al"), - "b.go": sets.NewString("al"), - "c.go": sets.NewString("charles"), +var ( + owners = map[string]string{ + "a.go": "1", + "b.go": "2", + "bb.go": "3", + "c.go": "4", - "e.go": sets.NewString("erick", "evan"), - "ee.go": sets.NewString("erick", "evan"), - }, - leafReviewers: map[string]sets.String{ - "a.go": sets.NewString("alice"), - "b.go": sets.NewString("bob"), - "bb.go": sets.NewString("bob", "ben"), - "c.go": sets.NewString("cole", "carl", "chad"), + "e.go": "5", + "ee.go": "5", + } + reviewers = map[string]sets.String{ + "a.go": sets.NewString("al"), + "b.go": sets.NewString("al"), + "c.go": sets.NewString("charles"), - "e.go": sets.NewString("erick", "ellen"), - "ee.go": sets.NewString("erick", "ellen"), - }, + "e.go": sets.NewString("erick", "evan"), + "ee.go": sets.NewString("erick", "evan"), } + leafReviewers = map[string]sets.String{ + "a.go": sets.NewString("alice"), + "b.go": sets.NewString("bob"), + "bb.go": sets.NewString("bob", "ben"), + "c.go": sets.NewString("cole", "carl", "chad"), - var testcases = []struct { + "e.go": sets.NewString("erick", "ellen"), + "ee.go": sets.NewString("erick", "ellen"), + } + testcases = []struct { name string filesChanged []string reviewerCount int @@ -189,6 +199,174 @@ func TestHandle(t *testing.T) { alternateExpectedRequested: []string{"bob"}, }, } +) + +// TestHandleWithExcludeApprovers tests that the handle function requests +// reviews from the correct number of unique users when ExcludeApprovers is +// true. +func TestHandleWithExcludeApproversOnlyReviewers(t *testing.T) { + foc := &fakeOwnersClient{ + owners: owners, + reviewers: reviewers, + leafReviewers: leafReviewers, + } + + for _, tc := range testcases { + fghc := newFakeGithubClient(tc.filesChanged) + pre := &github.PullRequestEvent{ + Number: 5, + PullRequest: github.PullRequest{User: github.User{Login: "author"}}, + Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"}, + } + if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), &tc.reviewerCount, nil, tc.maxReviewerCount, true, pre); err != nil { + t.Errorf("[%s] unexpected error from handle: %v", tc.name, err) + continue + } + + sort.Strings(fghc.requested) + sort.Strings(tc.expectedRequested) + sort.Strings(tc.alternateExpectedRequested) + if !reflect.DeepEqual(fghc.requested, tc.expectedRequested) { + if len(tc.alternateExpectedRequested) > 0 { + if !reflect.DeepEqual(fghc.requested, tc.alternateExpectedRequested) { + t.Errorf("[%s] expected the requested reviewers to be %q or %q, but got %q.", tc.name, tc.expectedRequested, tc.alternateExpectedRequested, fghc.requested) + } + continue + } + t.Errorf("[%s] expected the requested reviewers to be %q, but got %q.", tc.name, tc.expectedRequested, fghc.requested) + } + } +} + +// TestHandleWithoutExcludeApprovers verifies that behavior is the same +// when ExcludeApprovers is false and only approvers exist in the OWNERS files. +// The owners fixture and test cases should always be the same as the ones in +// TestHandleWithExcludeApprovers. +func TestHandleWithoutExcludeApproversNoReviewers(t *testing.T) { + foc := &fakeOwnersClient{ + owners: owners, + approvers: reviewers, + leafApprovers: leafReviewers, + } + + for _, tc := range testcases { + fghc := newFakeGithubClient(tc.filesChanged) + pre := &github.PullRequestEvent{ + Number: 5, + PullRequest: github.PullRequest{User: github.User{Login: "author"}}, + Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"}, + } + if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), &tc.reviewerCount, nil, tc.maxReviewerCount, false, pre); err != nil { + t.Errorf("[%s] unexpected error from handle: %v", tc.name, err) + continue + } + + sort.Strings(fghc.requested) + sort.Strings(tc.expectedRequested) + sort.Strings(tc.alternateExpectedRequested) + if !reflect.DeepEqual(fghc.requested, tc.expectedRequested) { + if len(tc.alternateExpectedRequested) > 0 { + if !reflect.DeepEqual(fghc.requested, tc.alternateExpectedRequested) { + t.Errorf("[%s] expected the requested reviewers to be %q or %q, but got %q.", tc.name, tc.expectedRequested, tc.alternateExpectedRequested, fghc.requested) + } + continue + } + t.Errorf("[%s] expected the requested reviewers to be %q, but got %q.", tc.name, tc.expectedRequested, fghc.requested) + } + } +} + +func TestHandleWithoutExcludeApproversMixed(t *testing.T) { + foc := &fakeOwnersClient{ + owners: map[string]string{ + "a.go": "1", + "b.go": "2", + "bb.go": "3", + "c.go": "4", + + "e.go": "5", + "ee.go": "5", + }, + approvers: map[string]sets.String{ + "a.go": sets.NewString("al"), + "b.go": sets.NewString("jeff"), + "c.go": sets.NewString("jeff"), + + "e.go": sets.NewString(), + "ee.go": sets.NewString("larry"), + }, + leafApprovers: map[string]sets.String{ + "a.go": sets.NewString("alice"), + "b.go": sets.NewString("brad"), + "c.go": sets.NewString("evan"), + + "e.go": sets.NewString("erick", "evan"), + "ee.go": sets.NewString("erick", "evan"), + }, + reviewers: map[string]sets.String{ + "a.go": sets.NewString("al"), + "b.go": sets.NewString(), + "c.go": sets.NewString("charles"), + + "e.go": sets.NewString("erick", "evan"), + "ee.go": sets.NewString("erick", "evan"), + }, + leafReviewers: map[string]sets.String{ + "a.go": sets.NewString("alice"), + "b.go": sets.NewString("bob"), + "bb.go": sets.NewString("bob", "ben"), + "c.go": sets.NewString("cole", "carl", "chad"), + + "e.go": sets.NewString("erick", "ellen"), + "ee.go": sets.NewString("erick", "ellen"), + }, + } + + var testcases = []struct { + name string + filesChanged []string + reviewerCount int + maxReviewerCount int + expectedRequested []string + alternateExpectedRequested []string + }{ + { + name: "1 file, 1 leaf reviewer, 1 leaf approver, 1 approver, request 3", + filesChanged: []string{"b.go"}, + reviewerCount: 3, + expectedRequested: []string{"bob", "brad", "jeff"}, + }, + { + name: "1 file, 1 leaf reviewer, 1 leaf approver, 1 approver, request 1, limit 1", + filesChanged: []string{"b.go"}, + reviewerCount: 1, + expectedRequested: []string{"bob"}, + }, + { + name: "2 file, 2 leaf reviewers, 1 parent reviewers, 1 leaf approver, 1 approver, request 5", + filesChanged: []string{"a.go", "b.go"}, + reviewerCount: 5, + expectedRequested: []string{"alice", "bob", "al", "brad", "jeff"}, + }, + { + name: "1 file, 1 leaf reviewer+approver, 1 reviewer+approver, request 3", + filesChanged: []string{"a.go"}, + reviewerCount: 3, + expectedRequested: []string{"alice", "al"}, + }, + { + name: "1 file, 2 leaf reviewers, request 2", + filesChanged: []string{"e.go"}, + reviewerCount: 2, + expectedRequested: []string{"erick", "ellen"}, + }, + { + name: "2 files, 2 leaf+parent reviewers, 1 parent reviewer, 1 parent approver, request 4", + filesChanged: []string{"e.go", "ee.go"}, + reviewerCount: 4, + expectedRequested: []string{"erick", "ellen", "evan", "larry"}, + }, + } for _, tc := range testcases { fghc := newFakeGithubClient(tc.filesChanged) pre := &github.PullRequestEvent{ @@ -196,7 +374,7 @@ func TestHandle(t *testing.T) { PullRequest: github.PullRequest{User: github.User{Login: "author"}}, Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"}, } - if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), &tc.reviewerCount, nil, tc.maxReviewerCount, pre); err != nil { + if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), &tc.reviewerCount, nil, tc.maxReviewerCount, false, pre); err != nil { t.Errorf("[%s] unexpected error from handle: %v", tc.name, err) continue } @@ -287,7 +465,7 @@ func TestHandleOld(t *testing.T) { PullRequest: github.PullRequest{User: github.User{Login: "author"}}, Repo: github.Repo{Owner: github.User{Login: "org"}, Name: "repo"}, } - if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), nil, &tc.reviewerCount, 0, pre); err != nil { + if err := handle(fghc, foc, logrus.WithField("plugin", pluginName), nil, &tc.reviewerCount, 0, false, pre); err != nil { t.Errorf("[%s] unexpected error from handle: %v", tc.name, err) continue } diff --git a/prow/plugins/plugins.go b/prow/plugins/plugins.go index 6d73e1133356..36a3a32ee531 100644 --- a/prow/plugins/plugins.go +++ b/prow/plugins/plugins.go @@ -199,6 +199,11 @@ type Blunderbuss struct { // reviews from. Selects reviewers based on file weighting. // This and request_count are mutually exclusive options. FileWeightCount *int `json:"file_weight_count,omitempty"` + // ExcludeApprovers controls whether approvers are considered to be + // reviewers. By default, approvers are considered as reviewers if + // insufficient reviewers are available. If ExcludeApprovers is true, + // approvers will never be considered as reviewers. + ExcludeApprovers bool `json:"exclude_approvers,omitempty"` } // Owners contains configuration related to handling OWNERS files.