Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: minimum reviewers on OWNERS file #1635

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ed3e68d
feat: minimum required reviewers on OWNERS
Skisocks Dec 26, 2024
f7287f4
test: update for minRequiredReviewers
Skisocks Dec 27, 2024
e175225
test: add tests for min required approvers
Skisocks Dec 28, 2024
b6f747e
feat: add required approvers text to message
Skisocks Jan 3, 2025
4fc16e2
test: add updated text to tests
Skisocks Jan 3, 2025
d96925a
refactor: simplify AreFilesApproved
Skisocks Jan 3, 2025
8473da9
fix: use the closest file to the change to get MinReviewers
Skisocks Jan 3, 2025
65062ce
test: add tests cases for MinReviewers
Skisocks Jan 3, 2025
e1459fd
fix: use the changed files to get the min required reviewers
Skisocks Jan 3, 2025
1928d45
feat: only set min reviewers if found in config
Skisocks Jan 5, 2025
4a8664e
fix: escape traverse if no min reviewers found
Skisocks Jan 5, 2025
f95ca28
fix: default to 0 if no min reviewers found, in keeping with current …
Skisocks Jan 5, 2025
0cf1e3f
test: add min reviewers to createFakeRepo
Skisocks Jan 5, 2025
a2fa460
test: approve handler for minimum reviewers
Skisocks Jan 5, 2025
0d5911c
chore: remove redundant function
Skisocks Jan 5, 2025
c242327
fix: default to 1 min reviewer if not found
Skisocks Jan 7, 2025
95470e0
fix: handle case where multiple reviewers but not all owners
Skisocks Jan 15, 2025
57c16b1
fix: text rendering in tests
Skisocks Jan 15, 2025
ef54099
test: add case for 2 required approvals from different OWNERS
Skisocks Jan 16, 2025
d55358e
test: add more coverage of minReviewers tests
Skisocks Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
test: add tests for min required approvers
Skisocks committed Jan 15, 2025
commit e17522556672a534e0b672a64b5223c2c06ba8aa
3 changes: 3 additions & 0 deletions pkg/plugins/approve/approve_test.go
Original file line number Diff line number Diff line change
@@ -149,6 +149,9 @@ type fakeRepo struct {
approverOwners map[string]string
}

func (fr fakeRepo) MinimumAmountOfRequiredReviewers(path string) int {
return 1
}
func (fr fakeRepo) Approvers(path string) sets.String {
return fr.approvers[path]
}
88 changes: 73 additions & 15 deletions pkg/plugins/approve/approvers/approvers_test.go
Original file line number Diff line number Diff line change
@@ -17,6 +17,7 @@ limitations under the License.
package approvers

import (
"github.com/stretchr/testify/assert"
"net/url"
"reflect"
"testing"
@@ -386,13 +387,21 @@ func TestIsApproved(t *testing.T) {
dApprovers := sets.NewString("David", "Dan", "Debbie")
eApprovers := sets.NewString("Eve", "Erin")
edcApprovers := eApprovers.Union(dApprovers).Union(cApprovers)
minApproversRoot := sets.NewString("Alice")
minApprovers2Required := sets.NewString("Alice", "Bob")
FakeRepoMap := map[string]sets.String{
"": rootApprovers,
"a": aApprovers,
"b": bApprovers,
"c": cApprovers,
"a/d": dApprovers,
"a/combo": edcApprovers,
"": rootApprovers,
"a": aApprovers,
"b": bApprovers,
"c": cApprovers,
"a/d": dApprovers,
"a/combo": edcApprovers,
"minReviewers": minApproversRoot,
"minReviewers/2Required": minApprovers2Required,
}
fakeMinReviewersMap := map[string]int{
"minReviewers": 1,
"minReviewers/2Required": 2,
}
tests := []struct {
testName string
@@ -406,7 +415,7 @@ func TestIsApproved(t *testing.T) {
filenames: []string{},
currentlyApproved: sets.NewString(),
testSeed: 0,
isApproved: true,
isApproved: false,
},
{
testName: "Single Root File PR Approved",
@@ -464,17 +473,66 @@ func TestIsApproved(t *testing.T) {
currentlyApproved: sets.NewString("Anne", "Ben", "Carol"),
isApproved: true,
},
{
testName: "C; 1 Approver",
filenames: []string{"c/test"},
testSeed: 0,
currentlyApproved: sets.NewString("Anne"),
isApproved: false,
},
{
testName: "Min Reviewers Root; 1 approval",
filenames: []string{"minReviewers/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Alice"),
isApproved: true,
},
{
testName: "Min Reviewers/2required; 1 approval",
filenames: []string{"minReviewers/2Required/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Alice"),
isApproved: false,
},
{
testName: "Min Reviewers/2required; 2 approvals",
filenames: []string{"minReviewers/2Required/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Alice", "Bob"),
isApproved: true,
},
{
testName: "Min Reviewers/2required; 2 approvals but not from required approvers",
filenames: []string{"minReviewers/2Required/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Tim", "Sally"),
isApproved: false,
},
{
testName: "Min Reviewers/2required & root; 1 approval",
filenames: []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Alice"),
isApproved: false,
},
{
testName: "Min Reviewers/2required & root; 2 approval",
filenames: []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
testSeed: 0,
currentlyApproved: sets.NewString("Alice", "Bob"),
isApproved: true,
},
}

for _, test := range tests {
testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
for approver := range test.currentlyApproved {
testApprovers.AddApprover(approver, "REFERENCE", false)
}
calculated := testApprovers.IsApproved()
if test.isApproved != calculated {
t.Errorf("Failed for test %v. Expected Approval Status: %v. Found %v", test.testName, test.isApproved, calculated)
}
t.Run(test.testName, func(t *testing.T) {
testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepoWithMinReviewers(FakeRepoMap, fakeMinReviewersMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
for approver := range test.currentlyApproved {
testApprovers.AddApprover(approver, "REFERENCE", false)
}
calculated := testApprovers.IsApproved()
assert.Equal(t, test.isApproved, calculated, "Failed for test %v", test.testName)
})
}
}

26 changes: 24 additions & 2 deletions pkg/plugins/approve/approvers/owners.go
Original file line number Diff line number Diff line change
@@ -45,6 +45,7 @@ type Repo interface {
LeafApprovers(path string) sets.String
FindApproverOwnersForFile(file string) string
IsNoParentOwners(path string) bool
MinimumAmountOfRequiredReviewers(path string) int
}

// Owners provides functionality related to owners of a specific code change.
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm aware of NoParentOwners I should be able to implement, this functionality just confused me 😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// It returns the highest value for minimum required approvers across all relevant OWNERS files.
func (o Owners) GetRequiredApproversCount() int {
requiredApprovers := 1
os := o.GetAllOwnersForFilesChanged()
for fn := range os {
if count := o.repo.MinimumAmountOfRequiredReviewers(fn); count > requiredApprovers {
requiredApprovers = count
}
}
return requiredApprovers
}

// GetLeafApprovers returns a map from ownersFiles -> people that are approvers in them (only the leaf)
func (o Owners) GetLeafApprovers() map[string]sets.String {
ownersToApprovers := map[string]sets.String{}
@@ -174,11 +188,17 @@ func (o Owners) GetSuggestedApprovers(reverseMap map[string]sets.String, potenti

// GetOwnersSet returns a set containing all the Owners files necessary to get the PR approved
func (o Owners) GetOwnersSet() sets.String {
owners := o.GetAllOwnersForFilesChanged()
o.removeSubdirs(owners)
return owners
}

// GetAllOwnersForFilesChanged returns the set of all owners for the files changed in the PR
func (o Owners) GetAllOwnersForFilesChanged() sets.String {
owners := sets.NewString()
for _, fn := range o.filenames {
owners.Insert(o.repo.FindApproverOwnersForFile(fn))
}
o.removeSubdirs(owners)
return owners
}

@@ -501,7 +521,9 @@ func (ap Approvers) GetCCs() []string {
// the PR are approved. If this returns true, the PR may still not be fully approved depending
// on the associated issue requirement
func (ap Approvers) AreFilesApproved() bool {
return ap.UnapprovedFiles().Len() == 0
currentApproversCount := ap.GetCurrentApproversSet().Len()
requiredApproversCount := ap.owners.GetRequiredApproversCount()
return ap.UnapprovedFiles().Len() == 0 && currentApproversCount >= requiredApproversCount
}

// RequirementsMet returns a bool indicating whether the PR has met all approval requirements:
14 changes: 14 additions & 0 deletions pkg/plugins/approve/approvers/owners_test.go
Original file line number Diff line number Diff line change
@@ -35,6 +35,14 @@ type FakeRepo struct {
approversMap map[string]sets.String
leafApproversMap map[string]sets.String
noParentOwnersMap map[string]bool
minReviewersMap map[string]int
}

func (f FakeRepo) MinimumAmountOfRequiredReviewers(path string) int {
if out, ok := f.minReviewersMap[path]; ok {
return out
}
return 1
}

func (f FakeRepo) Approvers(path string) sets.String {
@@ -86,6 +94,12 @@ func createFakeRepo(la map[string]sets.String) FakeRepo {
return FakeRepo{approversMap: a, leafApproversMap: la}
}

func createFakeRepoWithMinReviewers(la map[string]sets.String, ma map[string]int) FakeRepo {
fr := createFakeRepo(la)
fr.minReviewersMap = ma
return fr
}

func setToLower(s sets.String) sets.String {
lowered := sets.NewString()
for _, elem := range s.List() {