Skip to content

Commit

Permalink
Suggest approvers if reviewers aren't available
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@grantr
grantr committed May 25, 2018
1 parent bfcda98 commit 48d7aae
Show file tree
Hide file tree
Showing 3 changed files with 260 additions and 37 deletions.
54 changes: 47 additions & 7 deletions prow/plugins/blunderbuss/blunderbuss.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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
}
Expand All @@ -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))
}
Expand Down
238 changes: 208 additions & 30 deletions prow/plugins/blunderbuss/blunderbuss_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
Expand All @@ -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
Expand Down Expand Up @@ -189,14 +199,182 @@ 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{
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, 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
}
Expand Down Expand Up @@ -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
}
Expand Down
Loading

0 comments on commit 48d7aae

Please sign in to comment.