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
fix: use the changed files to get the min required reviewers
Skisocks committed Jan 15, 2025

Verified

This commit was signed with the committer’s verified signature.
jnatten Jonas Natten
commit e1459fd8a0f858e4c3150ec2b854f2d1609b10be
4 changes: 2 additions & 2 deletions pkg/plugins/approve/approvers/approvers_test.go
Original file line number Diff line number Diff line change
@@ -400,8 +400,8 @@ func TestIsApproved(t *testing.T) {
"minReviewers/2Required": minApprovers2Required,
}
fakeMinReviewersMap := map[string]int{
"minReviewers": 1,
"minReviewers/2Required": 2,
"minReviewers/test.go": 1,
"minReviewers/2Required/test.go": 2,
}
tests := []struct {
testName string
6 changes: 3 additions & 3 deletions pkg/plugins/approve/approvers/owners.go
Original file line number Diff line number Diff line change
@@ -78,8 +78,7 @@ func (o Owners) GetApprovers() map[string]sets.String {
// 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 {
for _, fn := range o.filenames {
if count := o.repo.MinimumAmountOfRequiredReviewers(fn); count > requiredApprovers {
requiredApprovers = count
}
@@ -525,7 +524,8 @@ 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 && ap.GetRemainingRequiredApprovers() <= 0
rr := ap.GetRemainingRequiredApprovers()
return ap.UnapprovedFiles().Len() == 0 && rr <= 0
}

// RequirementsMet returns a bool indicating whether the PR has met all approval requirements: