feat: minimum reviewers on OWNERS file #1635
Conversation
|
[jx-info] Hi, we've detected that the pipelines in this repository are using a syntax that will soon be deprecated. |
|
/cc @msvticket |
Skisocks
changed the title
| @@ -73,6 +74,19 @@ func (o Owners) GetApprovers() map[string]sets.String { | |||
| return ownersToApprovers | |||
| } | |||
|
|
|||
| // GetRequiredApproversCount returns the amount of approvers required to get a PR approved. | |||
There was a problem hiding this comment.
This definition of GetRequiredApproversCount is not sufficient since it doesn't check whether the approvers are the correct ones for the files. Actually this function and the use of it should be replaced with a check in UnapprovedFiles where you should replace
if len(approvers) == 0 {
with
if len(approvers) < ap.owners.repo.MinimumReviewersForFile(fn) {
Or some other equivalent change.
GetFiles should get the equivalent and then using this TestGetFiles should get test cases for minimum_reviewers.
There was a problem hiding this comment.
Well, after testing a bit myself this suggestion isn't totally sufficient. The reason is that the keys of the map returned by GetFilesApprovers is "normalised" to each directory where approvers are set for the changed files. So the test "Min Reviewers/2required & root; 1 approval" started to fail since minimum_reviews are set at a deeper path than approvers are. Hopefully you can find a better solution...
There was a problem hiding this comment.
@msvticket Been having difficulties implementing this as GetOwnersSet() (https://github.com/spring-financial-group/lighthouse/blob/main/pkg/plugins/approve/approvers/owners.go#L176) doesn't seem to return the OWNERS files that I'm expecting.
o.removeSubdirs() means that the highest level OWNERS file will always be returned for a given change, although I've now noticed that the NoParentOwners field allows you to override this. To me NoParentOwners should be the default behaviour (in fact this is how I thought it worked 😂), I'm wondering when you would ever not want this to be the case?
There was a problem hiding this comment.
Now I'm aware of NoParentOwners I should be able to implement, this functionality just confused me 😂
There was a problem hiding this comment.
I have to admit that I haven't been aware of no_parent_owners either.
Maybe you have found this already, but a function you probably need to change is Empty in repoowners.go
pkg/repoowners/repoowners.go
Outdated
| } | ||
| if foundMinReviewer == nil { | ||
| // If we didn't find a minimum reviewer count, default to 0 | ||
| return 0 |
There was a problem hiding this comment.
Shouldn't this be 1 to match current behaviour?
| return 0 | |
| return 1 |
pkg/plugins/approve/approve_test.go
Outdated
| files: []string{"d/d.go"}, | ||
| comments: []*scm.Comment{ | ||
| newTestComment("derek", "/approve"), | ||
| newTestComment("alice", "/approve"), |
There was a problem hiding this comment.
This test should have failed since alice should not be counted as an approver for this file. As this is implemented now anybody that writes an '/approve' comment in the PR will be counted as an approver, without being being listed in OWNERS file. See
https://dashboard-jx.infra.jenkins-x.rocks/ns-jx/jenkins-x/lighthouse/PR-1638/1 for my test where changing alice to anybody still makes the test succeed.
Keep this test case, but expect the PR to not be approved. Add more test cases covering the case where there are the correct approvers. Preferably add a case with different files covered by different OWNERS as well.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Skisocks
force-pushed
the
feat/min-reviewers
branch
from
a5d5669 to
57c16b1
Compare
Skisocks
force-pushed
the
feat/min-reviewers
branch
from
1ea7ae0 to
a30b0f2
Compare
Skisocks
force-pushed
the
feat/min-reviewers
branch
from
a30b0f2 to
d55358e
Compare
This PR adds the ability to set a minimum amount of reviewers required to Approve a pull request.
There are cases where certain directories in a repository are more sensitive than others and therefore require more stringent review procedures. For example, in the cluster definitions repository you may want a minimum of two approvals on your production namespace but only one approval on your staging namespace.
This is currently semi-achievable on GitHub by defining branch protections that require a min of two reviewers before merging, issues with this are that:
Logic Changes
A new field
minimum_reviewershas been added to OWNERS file, this int defines how many reviewers are required to Approve that OWNERS file.Identifying the relevant OWNERS file for a given changed file is done by starting at the changed file and moving up directories until an OWNERS file is found, the first one found defines the minimum reviewers required for that changed file. E.g. if
pkg/OWNERShas 2 minimum reviewers andpkg/util/OWNERShas 3 minimum reviewers this will return 3 for the pathpkg/util/sets/file.go.Identifying the minimum reviewers required to Approve an entire PR is done by taking that maximum
minimum_reviewersfound for all the files changed. I.e. the minimum amount of reviewers required for a PR will always be the highest value ofminimum_reviewersfound for all the files changed.This min reviewers logic works in conjuntion with the existing Approved/Unapproved OWNERS file logic, both have to be satisfied for a PR to be Approved.
To not break existing tests, if no
minimum_reviewersare found for the changed files then theminimum_reviewersis 0, effectively defaulting to the existing logic for approvers. I had thought that this should default to 1 min reviewer but decided it better to not affect exisiting repositories.Testing
Updated and added additional unit tests.
Ran this PR image in a cluster and is working as expected:
