Fallback to approvers as reviewers #7742

grantr · 2018-04-17T21:36:24Z

If the newly added blunderbuss option ~~ApproverFallback is true~~ ExcludeApprovers is false, the reviewer selection method will attempt to add approvers to the list if there aren't enough qualifying reviewers to satisfy the requested reviewer count.

While the implementation works and is tested with mixed approver and reviewer lists, this feature is most useful for projects that don't have a reviewer role and thus have OWNERS files containing only approver lists. Those projects ~~should now enable the ApproverFallback option~~ can rely on the default to have Blunderbuss select reviewers from approver lists.

/kind feature
/area prow

/assign @cjwagner
/cc @rsdcastro @fejta

Fixes #7691

fejta · 2018-04-18T07:59:58Z

Can we just make the review list the union of approvers and reviewers?

0xmichalis · 2018-04-18T09:00:28Z

I would argue for falling back to approvers by default (no config option).

stevekuznetsov · 2018-04-18T15:26:01Z

prow/plugins/blunderbuss/blunderbuss.go

@@ -61,6 +61,9 @@ func helpProvider(config *plugins.Configuration, enabledRepos []string) (*plugin
 }

 type ownersClient interface {


Seems like you should provide a different implementation of the ownersClient that returns approvers instead of duplicating methods

👍 I'll experiment with that approach.

@stevekuznetsov RFAL.

stevekuznetsov · 2018-04-18T15:26:33Z

/ok-to-test

fejta · 2018-05-12T18:12:29Z

Please get this to pass tests?

grantr · 2018-05-13T20:33:04Z

I haven't had time to work on this recently. :( If someone else wants to pick it up, feel free. Otherwise I'll try to carve out some time next week.

…

On Sat, May 12, 2018, 11:12 AM Erick Fejta ***@***.***> wrote: Please get this to pass tests? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAACqJVBbD7g2N3GvFYQ0hL-PlnljHIWks5txyYPgaJpZM4TZFiE> .

grantr · 2018-05-16T23:31:40Z

Can we just make the review list the union of approvers and reviewers?

I would argue for falling back to approvers by default (no config option).

Where should the fallback live? Should the behavior of repoowners.RepoOwners change to always fallback by default? Should blunderbuss wrap RepoOwners with its specific union implementation?

fejta · 2018-05-17T21:27:52Z

If we make the reviewer set the combination of everyone in reviewers and approvers then we do not need a fallback option, right?

cjwagner · 2018-05-18T00:09:33Z

If we make the reviewer set the combination of everyone in reviewers and approvers then we do not need a fallback option, right?

I don't think we should do that. We want to prioritize using the reviewers over the approvers and only use the approvers as a last resort if no reviewers are found. If we union the approvers and reviewers we can't distinguish them.

I think the fallback belongs in blunderbuss. Not all potential consumers of OWNERS file data from repoowners would want to have approvers returned in place of reviewers if reviewers are absent.

grantr · 2018-05-19T00:05:06Z

I removed the GetApprovers method and replaced it with an implementation that delegates to Approver methods. The existing ownersClient was renamed to reviewersClient:

type reviewersClient interface {
	FindReviewersOwnersForFile(path string) string
	Reviewers(path string) sets.String
	LeafReviewers(path string) sets.String
}

ownersClient was extended to include those methods plus the Approver methods:

type ownersClient interface {
	reviewersClient
	FindApproverOwnersForFile(path string) string
	Approvers(path string) sets.String
	LeafApprovers(path string) sets.String
}

That allows this implementation of reviewersClient to be passed to GetReviewers:

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)
}

grantr · 2018-05-19T00:07:29Z

What's the consensus on whether this should be enabled by default? Seems like @fejta, @Kargakis, and @cblecker think it should be the default. @cjwagner WDYT?

cblecker · 2018-05-19T00:36:54Z

Yes to this as the default, provided that "no" we don't just union reviewers and approvers. Agree 100% with @cjwagner's comment. I haven't reviewed the PR/implementation, but just an opinion from the experience POV.

fejta · 2018-05-19T01:25:21Z

I disagree that approvers should not share the burden of reviewing code. People who do not review the code should not be approving the code.

The intent of the reviewer system is to support overloaded code owners, not create a caste system where as soon as we add a reviewer the approvers are forever freed from the burden of doing initial reviews.

cblecker · 2018-05-19T01:48:27Z

Prow as a software product doesn’t need to dictate that, though. That situation would be solved by having that user in both the reviewer and the approver section (what we do in most places anyways).

This allows us to have the flexibility to allow a particular overloaded approver to drop from the frontline pool without revoking their approval rights.

grantr · 2018-05-24T23:05:15Z

I changed the name of the config option to ExcludeApprovers so the default behavior is now to fall back to approvers when necessary. I also cleaned up the duplicate fixtures in the tests so there's only one copy of each test case. Minor unrelated change: a warning is now logged when maxReviewers limits the number of reviewers requested, because I found that useful when debugging tests. RFAL.

@fejta I agree with you that making this explicit in OWNERS is probably a better solution, but a much more extensive one.

cblecker · 2018-05-25T18:52:21Z

On this PR: it looks good to me. Falling back seems like a sane default. Should probably squash out commits before merge.

On the social friction bikeshed: I agree from a healthy ecosystem point of view, but I'm not convinced that embedding that prow is needed. Maintaining healthy reviewer/approver pools (ensuring reviewers and approvers are active and responsive) is more critical to me.

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.

grantr · 2018-05-25T20:01:50Z

Rebased.

fejta

/lgtm
/hold

I remain convinced that combining approvers and reviewers into the reviewer set is the right default, but we don't need to resolve this conflict in this PR. And falling back to approvers is certainly better than the current situation.

Thanks!

fejta · 2018-05-29T18:07:10Z

prow/plugins/blunderbuss/blunderbuss.go

@@ -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)


should this only warn if excludeApprovers?

I moved this to warn after fallback to approvers (if enabled). WDYT?

fejta · 2018-05-29T18:07:32Z

prow/plugins/blunderbuss/blunderbuss.go

 		}
 	}

 	if len(reviewers) > 0 {
 		if maxReviewers > 0 && len(reviewers) > maxReviewers {
+			log.Warnf("Limiting request of %d reviewers to %d maxReviewers.", len(reviewers), maxReviewers)


This seems more like info to me

Agreed, fixed.

grantr

Thanks for the review @fejta! RFAL.

grantr · 2018-06-05T18:56:19Z

prow/plugins/blunderbuss/blunderbuss.go

@@ -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)


I moved this to warn after fallback to approvers (if enabled). WDYT?

grantr · 2018-06-05T18:57:01Z

prow/plugins/blunderbuss/blunderbuss.go

 		}
 	}

 	if len(reviewers) > 0 {
 		if maxReviewers > 0 && len(reviewers) > maxReviewers {
+			log.Warnf("Limiting request of %d reviewers to %d maxReviewers.", len(reviewers), maxReviewers)


Agreed, fixed.

grantr · 2018-06-05T20:36:59Z

/retest

grantr · 2018-06-05T20:37:15Z

/retest

fejta · 2018-06-05T22:18:09Z

/lgtm

k8s-ci-robot · 2018-06-05T22:18:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, grantr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [fejta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta · 2018-06-05T22:19:50Z

/hold cancel

grantr · 2018-06-05T22:21:01Z

Should this be announced on any mailing lists, since it changes default behavior?

fejta · 2018-06-05T22:24:31Z

We need to build and deploy a new prow before this takes effect.

Could you add a blurb to https://github.com/kubernetes/test-infra/blob/master/prow/ANNOUNCEMENTS.md?

Added in kubernetes#7742.

fejta · 2018-06-05T22:53:44Z

Sent a note to https://groups.google.com/d/msg/kubernetes-sig-contribex/HnOdQ2ChBR4/8QSFCyGKAgAJ

grantr requested review from cjwagner and stevekuznetsov as code owners April 17, 2018 21:36

k8s-ci-robot assigned cjwagner Apr 17, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. area/prow Issues or PRs related to prow labels Apr 17, 2018

k8s-ci-robot requested review from fejta and rsdcastro April 17, 2018 21:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2018

grantr mentioned this pull request Apr 17, 2018

Enable "blunderbuss" plugin knative/serving#630

Closed

stevekuznetsov requested changes Apr 18, 2018

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2018

grantr force-pushed the blunderbuss-only-approvers branch from e6166ac to 6a7566f Compare May 16, 2018 23:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2018

grantr mentioned this pull request May 18, 2018

tide should merge elafros PRs automatically knative/serving#543

Closed

grantr changed the title ~~Optionally fallback to approvers as reviewers~~ Fallback to approvers as reviewers May 24, 2018

grantr force-pushed the blunderbuss-only-approvers branch from ff84f72 to 48d7aae Compare May 25, 2018 20:01

fejta reviewed May 29, 2018

View reviewed changes

k8s-ci-robot assigned fejta May 29, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 29, 2018

Clean up reviewer count logging

d54576b

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

grantr commented Jun 5, 2018

View reviewed changes

grantr closed this Jun 5, 2018

grantr reopened this Jun 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2018

k8s-ci-robot merged commit aa73004 into kubernetes:master Jun 5, 2018

grantr deleted the blunderbuss-only-approvers branch June 5, 2018 22:21

grantr added a commit to grantr/test-infra that referenced this pull request Jun 5, 2018

Announce blunderbuss behavior update

9b21039

Added in kubernetes#7742.

grantr mentioned this pull request Jun 5, 2018

Announce blunderbuss behavior update #8269

Merged

		@@ -61,6 +61,9 @@ func helpProvider(config plugins.Configuration, enabledRepos []string) (plugin
		}

		type ownersClient interface {

Fallback to approvers as reviewers #7742

Fallback to approvers as reviewers #7742

Conversation

grantr commented Apr 17, 2018 • edited Loading

fejta commented Apr 18, 2018

0xmichalis commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Apr 18, 2018

fejta commented May 12, 2018

grantr commented May 13, 2018 via email

grantr commented May 16, 2018

fejta commented May 17, 2018

cjwagner commented May 18, 2018

grantr commented May 19, 2018

grantr commented May 19, 2018

cblecker commented May 19, 2018

fejta commented May 19, 2018 • edited Loading

cblecker commented May 19, 2018

grantr commented May 24, 2018

cblecker commented May 25, 2018

grantr commented May 25, 2018

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantr commented Jun 5, 2018

grantr commented Jun 5, 2018

fejta commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

fejta commented Jun 5, 2018

grantr commented Jun 5, 2018

fejta commented Jun 5, 2018

fejta commented Jun 5, 2018

grantr commented Apr 17, 2018 •

edited

Loading

fejta commented May 19, 2018 •

edited

Loading