diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index cf1e0895f..88de22a7c 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -18,6 +18,8 @@ package approve
import (
"fmt"
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
"net/url"
"os"
"reflect"
@@ -147,8 +149,15 @@ func newFakeSCMProviderClient(hasLabel, humanApproved, labelComments bool, files
type fakeRepo struct {
approvers, leafApprovers map[string]sets.String
approverOwners map[string]string
+ minimumReviewers map[string]int
}
+func (fr fakeRepo) MinimumReviewersForFile(path string) int {
+ if n, ok := fr.minimumReviewers[path]; ok {
+ return n
+ }
+ return 1
+}
func (fr fakeRepo) Approvers(path string) sets.String {
return fr.approvers[path]
}
@@ -244,7 +253,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -610,6 +620,7 @@ Approvers can cancel approval by writing `+"`/approve cancel`"+` in a comment
Approval requirements bypassed by manually added approval.
This pull-request has been approved by:
+The changes made require 1 more approval(s).
The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).
@@ -674,7 +685,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
comments: []*scm.Comment{
newTestComment("k8s-ci-robot", `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **alice**
You can assign the PR to them by writing `+"`/assign @alice`"+` in a comment when ready.
@@ -752,7 +764,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -822,7 +835,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -860,7 +874,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -936,7 +951,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -971,7 +987,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
expectComment: true,
expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
-This pull-request has been approved by:
+This pull-request has been approved by:
+The changes made require 1 more approval(s).
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **cjwagner**
You can assign the PR to them by writing ` + "`/assign @cjwagner`" + ` in a comment when ready.
@@ -1127,6 +1144,148 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
`,
},
+ {
+ name: "2 minimum reviewers - no approvals",
+ hasLabel: false,
+ files: []string{"d/d.go"},
+ comments: []*scm.Comment{},
+ reviews: []*scm.Review{},
+ selfApprove: false,
+ needsIssue: false,
+ lgtmActsAsApprove: false,
+ reviewActsAsApprove: false,
+ githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},
+
+ expectDelete: false,
+ expectToggle: false,
+ expectComment: true,
+ expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
+
+This pull-request has been approved by:
+The changes made require 2 more approval(s).
+To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
+You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.
+
+The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).
+
+
+Needs approval from an approver in each of these files:
+
+- **[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)**
+
+Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
+
+`,
+ },
+ {
+ name: "2 minimum reviewers - 1 approval",
+ hasLabel: false,
+ files: []string{"d/d.go"},
+ comments: []*scm.Comment{
+ newTestComment("derek", "/approve"),
+ },
+ reviews: []*scm.Review{},
+ selfApprove: false,
+ needsIssue: false,
+ lgtmActsAsApprove: false,
+ reviewActsAsApprove: false,
+ githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},
+
+ expectDelete: false,
+ expectToggle: false,
+ expectComment: true,
+ expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
+
+This pull-request has been approved by: *[derek](<> "Approved")*
+The changes made require 1 more approval(s).
+To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
+You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.
+
+The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).
+
+
+Needs approval from an approver in each of these files:
+
+- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek]
+
+Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
+
+`,
+ },
+ {
+ name: "2 minimum reviewers - 2 approval",
+ hasLabel: false,
+ files: []string{"d/d.go"},
+ comments: []*scm.Comment{
+ newTestComment("derek", "/approve"),
+ newTestComment("jerry", "/approve"),
+ },
+ reviews: []*scm.Review{},
+ selfApprove: false,
+ needsIssue: false,
+ lgtmActsAsApprove: false,
+ reviewActsAsApprove: false,
+ githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},
+
+ expectDelete: false,
+ expectToggle: true,
+ expectComment: true,
+ expectedComment: `[APPROVALNOTIFIER] This PR is **APPROVED**
+
+This pull-request has been approved by: *[derek](<> "Approved")*, *[jerry](<> "Approved")*
+
+The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).
+
+The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)
+
+
+Needs approval from an approver in each of these files:
+
+- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek,jerry]
+
+Approvers can indicate their approval by writing ` + "`/approve` " + `in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel` " + `in a comment
+
+`,
+ },
+ {
+ name: "2 minimum reviewers - 1 approval, 1 not registered approval",
+ hasLabel: false,
+ files: []string{"d/d.go"},
+ comments: []*scm.Comment{
+ newTestComment("derek", "/approve"),
+ newTestComment("alice", "/approve"),
+ },
+ reviews: []*scm.Review{},
+ selfApprove: false,
+ needsIssue: false,
+ lgtmActsAsApprove: false,
+ reviewActsAsApprove: false,
+ githubLinkURL: &url.URL{Scheme: "https", Host: "github.com"},
+
+ expectDelete: false,
+ expectToggle: false,
+ expectComment: true,
+ expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
+
+This pull-request has been approved by: *[alice](<> "Approved")*, *[derek](<> "Approved")*
+To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **derek**
+You can assign the PR to them by writing ` + "`/assign @derek`" + ` in a comment when ready.
+
+The full list of commands accepted by this bot can be found [here](https://jenkins-x.io/v3/develop/reference/chatops/?repo=org%2Frepo).
+
+
+Needs approval from an approver in each of these files:
+
+- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek]
+
+Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
+Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
+
+`,
+ },
}
fr := fakeRepo{
@@ -1134,17 +1293,23 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
"a": sets.NewString("alice"),
"a/b": sets.NewString("alice", "bob"),
"c": sets.NewString("cblecker", "cjwagner"),
+ "d": sets.NewString("derek", "jerry"),
},
leafApprovers: map[string]sets.String{
"a": sets.NewString("alice"),
"a/b": sets.NewString("bob"),
"c": sets.NewString("cblecker", "cjwagner"),
+ "d": sets.NewString("derek", "jerry"),
},
approverOwners: map[string]string{
"a/a.go": "a",
"a/aa.go": "a",
"a/b/b.go": "a/b",
"c/c.go": "c",
+ "d/d.go": "d",
+ },
+ minimumReviewers: map[string]int{
+ "d": 2,
},
}
@@ -1158,7 +1323,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
rsa := !test.selfApprove
irs := !test.reviewActsAsApprove
- if err := handle(
+ err := handle(
logrus.WithField("plugin", "approve"),
fakeClient,
fr,
@@ -1179,9 +1344,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
author: "cjwagner",
assignees: []scm.User{{Login: "spxtr"}},
},
- ); err != nil {
- t.Errorf("[%s] Unexpected error handling event: %v.", test.name, err)
- }
+ )
+ require.NoError(t, err)
fakeLabel := fmt.Sprintf("org/repo#%v:approved", prNumber)
@@ -1223,12 +1387,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
len(fspc.PullRequestCommentsAdded),
)
} else if expect, got := fmt.Sprintf("org/repo#%v:", prNumber)+test.expectedComment, fspc.PullRequestCommentsAdded[0]; test.expectedComment != "" && got != expect {
- t.Errorf(
- "[%s] Expected the created notification to be:\n%s\n\nbut got:\n%s\n\n",
- test.name,
- expect,
- got,
- )
+ assert.Equal(t, expect, got, "actual notification does not equal expected")
}
} else {
if len(fspc.PullRequestCommentsAdded) != 0 {
@@ -1262,12 +1421,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
}
}
if test.expectToggle != toggled {
- t.Errorf(
- "[%s] Expected 'approved' label toggled: %t, but got %t.",
- test.name,
- test.expectToggle,
- toggled,
- )
+ assert.Equal(t, test.expectToggle, toggled, "actual toggle state does not equal expected")
}
})
}
@@ -1306,6 +1460,10 @@ func (fro fakeRepoOwners) RequiredReviewers(path string) sets.String {
return sets.NewString()
}
+func (fro fakeRepoOwners) MinimumReviewersForFile(path string) int {
+ return 1
+}
+
func TestHandleGenericComment(t *testing.T) {
tests := []struct {
name string
diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index 400cfeec2..98544a54a 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -17,6 +17,7 @@ limitations under the License.
package approvers
import (
+ "github.com/stretchr/testify/assert"
"net/url"
"reflect"
"testing"
@@ -394,6 +395,7 @@ func TestIsApproved(t *testing.T) {
"a/d": dApprovers,
"a/combo": edcApprovers,
}
+
tests := []struct {
testName string
filenames []string
@@ -464,17 +466,330 @@ 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,
+ },
}
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) {
+ fakeRepo := createFakeRepo(FakeRepoMap)
+ testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: fakeRepo, 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)
+ })
+ }
+}
+
+func TestIsApprovedWithMinReviewers(t *testing.T) {
+ tests := []struct {
+ testName string
+ filenames []string
+ leafApprovers map[string]sets.String
+ minReviewersMap map[string]int
+ noParentOwnersMap map[string]bool
+ currentlyApproved sets.String
+ testSeed int64
+ isApproved bool
+ }{
+ {
+ testName: "1 min reviewer: 1 approval",
+ filenames: []string{"f/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 1,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice"),
+ isApproved: true,
+ },
+ {
+ testName: "2 min reviewers: 1 approval",
+ filenames: []string{"f/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice"),
+ isApproved: false,
+ },
+ {
+ testName: "2 min reviewers: 2 approvals from OWNERS",
+ filenames: []string{"f/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Bob"),
+ isApproved: true,
+ },
+ {
+ testName: "2 min reviewers: 1 approve from OWNERS, 1 not",
+ filenames: []string{"f/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Bob", "Tim"),
+ isApproved: false,
+ },
+ {
+ testName: "Multi-file: 2 min reviewers: everyone approves",
+ filenames: []string{"f/test.go", "g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Bob", "Tom", "Harry"),
+ isApproved: true,
+ },
+ {
+ testName: "Multi-file: 2 min reviewers: 1 approval from each file",
+ filenames: []string{"f/test.go", "g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom"),
+ isApproved: false,
+ },
+ {
+ testName: "Multi-file: 2 min reviewers: f fully approved, g not",
+ filenames: []string{"f/test.go", "g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Bob", "Tom"),
+ isApproved: false,
+ },
+ {
+ testName: "Nested-files: equal min reviewers: everyone approves",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Bob", "Tom", "Harry"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: equal min reviewers: 1 approval from each file",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom"),
+ isApproved: false,
+ },
+ {
+ testName: "Nested-files: equal min reviewers: f fully approved, f/g not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Bob", "Tom"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: equal min reviewers: f/g fully approved, f not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Bob"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Tom", "Harry", "Alice"),
+ isApproved: false,
+ },
+ {
+ testName: "Nested-files: top-level less min reviewers: everyone approves",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 1,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom", "Harry"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: top-level less min reviewers: 1 approval from each file",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 1,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: top-level less min reviewers: f fully approved, f/g not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 1,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: top-level less min reviewers: f/g fully approved, f not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice"),
+ "f/g": sets.NewString("Tom", "Harry"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 1,
+ "f/g": 2,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Tom", "Harry"),
+ isApproved: false,
+ },
+ {
+ testName: "Nested-files: top-level more min reviewers: everyone approves",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Harry"),
+ "f/g": sets.NewString("Tom"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 1,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom", "Harry"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: top-level more min reviewers: 1 approval from each file",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Harry"),
+ "f/g": sets.NewString("Tom"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 1,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Tom"),
+ isApproved: false,
+ },
+ {
+ testName: "Nested-files: top-level more min reviewers: f fully approved, f/g not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Harry"),
+ "f/g": sets.NewString("Tom"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 1,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Alice", "Harry"),
+ isApproved: true,
+ },
+ {
+ testName: "Nested-files: top-level more min reviewers: f/g fully approved, f not",
+ filenames: []string{"f/test.go", "f/g/test.go"},
+ leafApprovers: map[string]sets.String{
+ "f": sets.NewString("Alice", "Harry"),
+ "f/g": sets.NewString("Tom"),
+ },
+ minReviewersMap: map[string]int{
+ "f": 2,
+ "f/g": 1,
+ },
+ testSeed: 0,
+ currentlyApproved: sets.NewString("Tom", "Harry"),
+ isApproved: false,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.testName, func(t *testing.T) {
+ fakeRepo := createFakeRepo(test.leafApprovers)
+ fakeRepo.minReviewersMap = test.minReviewersMap
+ fakeRepo.noParentOwnersMap = test.noParentOwnersMap
+ testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: fakeRepo, 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)
+ })
}
}
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 4511edef5..2db2af326 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -45,6 +45,7 @@ type Repo interface {
LeafApprovers(path string) sets.String
FindApproverOwnersForFile(file string) string
IsNoParentOwners(path string) bool
+ MinimumReviewersForFile(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.
+// It finds the highest value for minimum required reviewers across all the changed files.
+// If no minimum reviewers are found then 0 is returned.
+func (o Owners) GetRequiredApproversCount() int {
+ var requiredApprovers int
+ for fn := range o.GetOwnersSet() {
+ if count := o.repo.MinimumReviewersForFile(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{}
@@ -162,7 +176,7 @@ func (o Owners) GetSuggestedApprovers(reverseMap map[string]sets.String, potenti
ap := NewApprovers(o)
for !ap.RequirementsMet() {
newApprover := findMostCoveringApprover(potentialApprovers, reverseMap, ap.UnapprovedFiles())
- if newApprover == "" {
+ if newApprover == "" || ap.GetCurrentApproversSet().Has(newApprover) {
o.log.Warnf("Couldn't find/suggest approvers for each files. Unapproved: %q", ap.UnapprovedFiles().List())
return ap.GetCurrentApproversSet()
}
@@ -429,13 +443,17 @@ func (ap Approvers) NoIssueApprovers() map[string]Approval {
func (ap Approvers) UnapprovedFiles() sets.String {
unapproved := sets.NewString()
for fn, approvers := range ap.GetFilesApprovers() {
- if len(approvers) == 0 {
+ if len(approvers) < ap.owners.repo.MinimumReviewersForFile(fn) {
unapproved.Insert(fn)
}
}
return unapproved
}
+func (ap Approvers) GetRemainingRequiredApprovers() int {
+ return ap.owners.GetRequiredApproversCount() - ap.GetCurrentApproversSet().Len()
+}
+
// GetFiles returns owners files that still need approval.
func (ap Approvers) GetFiles(baseURL *url.URL, owner, repo, branch, providerType string) []File {
allOwnersFiles := []File{}
@@ -637,6 +655,9 @@ Approval requirements bypassed by manually added approval.
{{end -}}
This pull-request has been approved by:{{range $index, $approval := .ap.ListApprovals}}{{if $index}}, {{else}} {{end}}{{$approval}}{{end}}
+{{- if (gt .ap.GetRemainingRequiredApprovers 0) }}
+The changes made require {{ .ap.GetRemainingRequiredApprovers }} more approval(s).
+{{- end }}
{{- if (and (not .ap.AreFilesApproved) (not (call .ap.ManuallyApproved))) }}
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign {{range $index, $cc := .ap.GetCCs}}{{if $index}}, {{end}}**{{$cc}}**{{end}}
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index 24abcc4b3..63a040de3 100644
--- a/pkg/plugins/approve/approvers/owners_test.go
+++ b/pkg/plugins/approve/approvers/owners_test.go
@@ -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) MinimumReviewersForFile(path string) int {
+ if out, ok := f.minReviewersMap[path]; ok {
+ return out
+ }
+ return 1
}
func (f FakeRepo) Approvers(path string) sets.String {
@@ -82,7 +90,6 @@ func createFakeRepo(la map[string]sets.String) FakeRepo {
}
}
}
-
return FakeRepo{approversMap: a, leafApproversMap: la}
}
diff --git a/pkg/plugins/lgtm/lgtm_test.go b/pkg/plugins/lgtm/lgtm_test.go
index c36c99e50..43feeb9b5 100644
--- a/pkg/plugins/lgtm/lgtm_test.go
+++ b/pkg/plugins/lgtm/lgtm_test.go
@@ -77,6 +77,7 @@ func (f *fakeRepoOwners) Approvers(path string) sets.String { return
func (f *fakeRepoOwners) LeafReviewers(path string) sets.String { return nil }
func (f *fakeRepoOwners) Reviewers(path string) sets.String { return f.reviewers[path] }
func (f *fakeRepoOwners) RequiredReviewers(path string) sets.String { return nil }
+func (f *fakeRepoOwners) MinimumReviewersForFile(path string) int { return 1 }
var approvers = map[string]sets.String{
"doc/README.md": {
diff --git a/pkg/repoowners/repoowners.go b/pkg/repoowners/repoowners.go
index eab5033e8..728a7d581 100644
--- a/pkg/repoowners/repoowners.go
+++ b/pkg/repoowners/repoowners.go
@@ -55,6 +55,7 @@ type Config struct {
Reviewers []string `json:"reviewers,omitempty"`
RequiredReviewers []string `json:"required_reviewers,omitempty"`
Labels []string `json:"labels,omitempty"`
+ MinimumReviewers *int `json:"minimum_reviewers,omitempty"`
}
// SimpleConfig holds options and Config applied to everything under the containing directory
@@ -158,6 +159,7 @@ type RepoOwner interface {
LeafReviewers(path string) sets.String
Reviewers(path string) sets.String
RequiredReviewers(path string) sets.String
+ MinimumReviewersForFile(path string) int
}
var _ RepoOwner = &RepoOwners{}
@@ -170,6 +172,7 @@ type RepoOwners struct {
reviewers map[string]map[*regexp.Regexp]sets.String
requiredReviewers map[string]map[*regexp.Regexp]sets.String
labels map[string]map[*regexp.Regexp]sets.String
+ minimumReviewers map[string]map[*regexp.Regexp]int
options map[string]dirOptions
baseDir string
@@ -387,6 +390,7 @@ func loadOwnersFrom(baseDir string, mdYaml bool, aliases RepoAliases, dirExclude
reviewers: make(map[string]map[*regexp.Regexp]sets.String),
requiredReviewers: make(map[string]map[*regexp.Regexp]sets.String),
labels: make(map[string]map[*regexp.Regexp]sets.String),
+ minimumReviewers: make(map[string]map[*regexp.Regexp]int),
options: make(map[string]dirOptions),
dirExcludes: dirExcludes,
@@ -554,6 +558,13 @@ func (o *RepoOwners) applyConfigToPath(path string, re *regexp.Regexp, config *C
}
o.labels[path][re] = sets.NewString(config.Labels...)
}
+
+ if config.MinimumReviewers != nil {
+ if o.minimumReviewers[path] == nil {
+ o.minimumReviewers[path] = make(map[*regexp.Regexp]int)
+ }
+ o.minimumReviewers[path][re] = *config.MinimumReviewers
+ }
}
func (o *RepoOwners) applyOptionsToPath(path string, opts dirOptions) {
@@ -671,6 +682,46 @@ func (o *RepoOwners) entriesForFile(path string, people map[string]map[*regexp.R
return out
}
+// MinimumReviewersForFile returns the minimum number of reviewers required to approve the requested file.
+// This is determined by the OWNERS file closest to the requested file.
+// If pkg/OWNERS has 2 minimum reviewers and pkg/util/OWNERS has 3 minimum reviewers this will return 3 for the path pkg/util/sets/file.go
+// If no minimum reviewers can be found then 0 is returned.
+func (o *RepoOwners) MinimumReviewersForFile(path string) int {
+ d := path
+ if !o.enableMDYAML || !strings.HasSuffix(path, ".md") {
+ // if path is a directory, this will remove the leaf directory, and returns "." for topmost dir
+ d = filepath.Dir(d)
+ d = canonicalize(d)
+ }
+
+ var foundMinReviewer *int
+ for {
+ relative, err := filepath.Rel(d, path)
+ if err != nil {
+ o.log.WithError(err).WithField("path", path).Errorf("Unable to find relative path between %q and path.", d)
+ foundMinReviewer = nil
+ break
+ }
+ for re, s := range o.minimumReviewers[d] {
+ if re == nil || re.MatchString(relative) {
+ foundMinReviewer = &s
+ break
+ }
+ }
+ if foundMinReviewer != nil || d == baseDirConvention {
+ // If we found a minimum reviewer count, or we've reached the base directory, break
+ break
+ }
+ d = filepath.Dir(d)
+ d = canonicalize(d)
+ }
+ if foundMinReviewer == nil {
+ // If we didn't find a minimum reviewer count, default to 0
+ return 1
+ }
+ return *foundMinReviewer
+}
+
// LeafApprovers returns a set of users who are the closest approvers to the
// requested file. If pkg/OWNERS has user1 and pkg/util/OWNERS has user2 this
// will only return user2 for the path pkg/util/sets/file.go
diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index 4969f8578..2bc583b44 100644
--- a/pkg/repoowners/repoowners_test.go
+++ b/pkg/repoowners/repoowners_test.go
@@ -18,6 +18,7 @@ package repoowners
import (
"fmt"
+ "github.com/stretchr/testify/assert"
"path/filepath"
"reflect"
"regexp"
@@ -58,6 +59,7 @@ reviewers:
- jakub
required_reviewers:
- ben
+minimum_reviewers: 2
labels:
- src-code`),
"src/dir/conformance/OWNERS": []byte(`options:
@@ -331,6 +333,7 @@ func TestLoadRepoOwners(t *testing.T) {
extraBranchesAndFiles map[string]map[string][]byte
expectedApprovers, expectedReviewers, expectedRequiredReviewers, expectedLabels map[string]map[string]sets.String
+ expectedMinRequiredReviewers map[string]map[string]int
expectedOptions map[string]dirOptions
}{
@@ -354,6 +357,9 @@ func TestLoadRepoOwners(t *testing.T) {
"": patternAll("EVERYTHING"),
"src/dir": patternAll("src-code"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -381,6 +387,9 @@ func TestLoadRepoOwners(t *testing.T) {
"": patternAll("EVERYTHING"),
"src/dir": patternAll("src-code"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -411,6 +420,9 @@ func TestLoadRepoOwners(t *testing.T) {
"src/dir": patternAll("src-code"),
"docs/file.md": patternAll("docs"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -444,6 +456,9 @@ func TestLoadRepoOwners(t *testing.T) {
"": patternAll("EVERYTHING"),
"src/dir": patternAll("src-code"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -476,6 +491,9 @@ func TestLoadRepoOwners(t *testing.T) {
"": patternAll("EVERYTHING"),
"src/dir": patternAll("src-code"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -503,6 +521,9 @@ func TestLoadRepoOwners(t *testing.T) {
"": patternAll("EVERYTHING"),
"src/dir": patternAll("src-code"),
},
+ expectedMinRequiredReviewers: map[string]map[string]int{
+ "src/dir": {"": 2},
+ },
expectedOptions: map[string]dirOptions{
"src/dir/conformance": {
NoParentOwners: true,
@@ -544,7 +565,7 @@ func TestLoadRepoOwners(t *testing.T) {
continue
}
- check := func(field string, expected map[string]map[string]sets.String, got map[string]map[*regexp.Regexp]sets.String) {
+ checkStringSet := func(field string, expected map[string]map[string]sets.String, got map[string]map[*regexp.Regexp]sets.String) {
converted := map[string]map[string]sets.String{}
for path, m := range got {
converted[path] = map[string]sets.String{}
@@ -560,10 +581,29 @@ func TestLoadRepoOwners(t *testing.T) {
t.Errorf("Expected %s to be:\n%+v\ngot:\n%+v.", field, expected, converted)
}
}
- check("approvers", test.expectedApprovers, ro.approvers)
- check("reviewers", test.expectedReviewers, ro.reviewers)
- check("required_reviewers", test.expectedRequiredReviewers, ro.requiredReviewers)
- check("labels", test.expectedLabels, ro.labels)
+
+ checkInt := func(field string, expected map[string]map[string]int, got map[string]map[*regexp.Regexp]int) {
+ converted := map[string]map[string]int{}
+ for path, m := range got {
+ converted[path] = map[string]int{}
+ for re, s := range m {
+ var pattern string
+ if re != nil {
+ pattern = re.String()
+ }
+ converted[path][pattern] = s
+ }
+ }
+ if !reflect.DeepEqual(expected, converted) {
+ t.Errorf("Expected %s to be:\n%+v\ngot:\n%+v.", field, expected, converted)
+ }
+ }
+
+ checkStringSet("approvers", test.expectedApprovers, ro.approvers)
+ checkStringSet("reviewers", test.expectedReviewers, ro.reviewers)
+ checkStringSet("required_reviewers", test.expectedRequiredReviewers, ro.requiredReviewers)
+ checkStringSet("labels", test.expectedLabels, ro.labels)
+ checkInt("min_required_reviewers", test.expectedMinRequiredReviewers, ro.minimumReviewers)
if !reflect.DeepEqual(test.expectedOptions, ro.options) {
t.Errorf("Expected options to be:\n%#v\ngot:\n%#v.", test.expectedOptions, ro.options)
}
@@ -858,3 +898,66 @@ func TestExpandAliases(t *testing.T) {
}
}
}
+
+func TestGetRequiredApproversCount(t *testing.T) {
+ const (
+ // No min reviewers
+ baseDir = ""
+ // Min reviewers set to 1
+ secondDir = "a"
+ // Min reviewers set to 2
+ thirdDir = "a/b"
+ // Min reviewers set to 3
+ fourthDir = "a/b/c"
+ )
+
+ ro := &RepoOwners{
+ minimumReviewers: map[string]map[*regexp.Regexp]int{
+ secondDir: {nil: 1},
+ thirdDir: {nil: 2},
+ fourthDir: {nil: 3},
+ },
+ options: map[string]dirOptions{
+ noParentsDir: {
+ NoParentOwners: true,
+ },
+ },
+ }
+ testCases := []struct {
+ name string
+ filePath string
+ expectedRequiredApprovers int
+ }{
+ {
+ name: "Modified Base Dir",
+ filePath: filepath.Join(baseDir, "main.go"),
+ expectedRequiredApprovers: 1,
+ },
+ {
+ name: "Modified Second Dir",
+ filePath: filepath.Join(secondDir, "main.go"),
+ expectedRequiredApprovers: ro.minimumReviewers[secondDir][nil],
+ },
+ {
+ name: "Modified Third Dir",
+ filePath: filepath.Join(thirdDir, "main.go"),
+ expectedRequiredApprovers: ro.minimumReviewers[thirdDir][nil],
+ },
+ {
+ name: "Modified Nested Dir Without OWNERS (default to fourth dir)",
+ filePath: filepath.Join(fourthDir, "d", "main.go"),
+ expectedRequiredApprovers: ro.minimumReviewers[fourthDir][nil],
+ },
+ {
+ name: "Modified Nonexistent Dir (default to Base Dir)",
+ filePath: filepath.Join("nonexistent", "main.go"),
+ expectedRequiredApprovers: 1,
+ },
+ }
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ actual := ro.MinimumReviewersForFile(tc.filePath)
+ assert.Equal(t, tc.expectedRequiredApprovers, actual)
+ })
+ }
+}