fix: handle case where multiple reviewers but not all owners

Author: Skisocks

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"
@@ -1197,8 +1199,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 
 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 
-You can assign the PR to them by writing ` + "`/assign `" + ` in a comment when ready.
+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).
 
@@ -1210,15 +1212,15 @@ Needs approval from an approver in each of these files:
 Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comment
 Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
 </details>
-<!-- META={"approvers":[]} -->`,
+<!-- META={"approvers":["derek"]} -->`,
 		},
 		{
 			name:     "2 minimum reviewers - 2 approval",
 			hasLabel: false,
 			files:    []string{"d/d.go"},
 			comments: []*scm.Comment{
 				newTestComment("derek", "/approve"),
-				newTestComment("alice", "/approve"),
+				newTestComment("jerry", "/approve"),
 			},
 			reviews:             []*scm.Review{},
 			selfApprove:         false,
@@ -1232,7 +1234,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectComment: true,
 			expectedComment: `[APPROVALNOTIFIER] This PR is **APPROVED**
 
-This pull-request has been approved by: *[alice](<> "Approved")*, *[derek](<> "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).
 
@@ -1241,27 +1243,63 @@ The pull request process is described [here](https://git.k8s.io/community/contri
 <details >
 Needs approval from an approver in each of these files:
 
-- ~~[d/OWNERS](https://github.com/org/repo/blob/master/d/OWNERS)~~ [derek]
+- ~~[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
 </details>
 <!-- META={"approvers":[]} -->`,
 		},
+		{
+			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).
+
+<details open>
+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
+</details>
+<!-- META={"approvers":["derek"]} -->`,
+		},
 	}
 
 	fr := fakeRepo{
 		approvers: map[string]sets.String{
 			"a":   sets.NewString("alice"),
 			"a/b": sets.NewString("alice", "bob"),
 			"c":   sets.NewString("cblecker", "cjwagner"),
-			"d":   sets.NewString("derek"),
+			"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"),
+			"d":   sets.NewString("derek", "jerry"),
 		},
 		approverOwners: map[string]string{
 			"a/a.go":   "a",
@@ -1271,7 +1309,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel` " + `in a commen
 			"d/d.go":   "d",
 		},
 		minimumReviewers: map[string]int{
-			"d/d.go": 2,
+			"d": 2,
 		},
 	}
 
@@ -1285,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,
@@ -1306,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)
 
@@ -1350,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 {
@@ -1389,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")
 			}
 		})
 	}

pkg/plugins/approve/approvers/approvers_test.go

@@ -400,9 +400,14 @@ func TestIsApproved(t *testing.T) {
 		"minReviewers/2Required": minApprovers2Required,
 	}
 	fakeMinReviewersMap := map[string]int{
-		"minReviewers/test.go":           1,
-		"minReviewers/2Required/test.go": 2,
+		"minReviewers":           1,
+		"minReviewers/2Required": 2,
 	}
+	fakeNoParentsOwnersMap := map[string]bool{
+		"minReviewers":           true,
+		"minReviewers/2Required": true,
+	}
+
 	tests := []struct {
 		testName          string
 		filenames         []string
@@ -522,11 +527,20 @@ func TestIsApproved(t *testing.T) {
 			currentlyApproved: sets.NewString("Alice", "Bob"),
 			isApproved:        true,
 		},
+		{
+			testName:          "Min Reviewers/2required & root; 1 approval, 1 not registered approver",
+			filenames:         []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
+			testSeed:          0,
+			currentlyApproved: sets.NewString("Alice", "Derek"),
+			isApproved:        false,
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, fakeMinReviewersMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
+			fakeRepo := createFakeRepo(FakeRepoMap, fakeMinReviewersMap)
+			fakeRepo.noParentOwnersMap = fakeNoParentsOwnersMap
+			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)
 			}

pkg/plugins/approve/approvers/owners.go

@@ -79,7 +79,7 @@ func (o Owners) GetApprovers() map[string]sets.String {
 // If no minimum reviewers are found then 0 is returned.
 func (o Owners) GetRequiredApproversCount() int {
 	var requiredApprovers int
-	for _, fn := range o.filenames {
+	for fn := range o.GetOwnersSet() {
 		if count := o.repo.MinimumReviewersForFile(fn); count > requiredApprovers {
 			requiredApprovers = count
 		}
@@ -176,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()
 		}
@@ -443,7 +443,7 @@ 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)
 		}
 	}
@@ -519,7 +519,7 @@ 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
+	return ap.UnapprovedFiles().Len() == 0
 }
 
 // RequirementsMet returns a bool indicating whether the PR has met all approval requirements:

pkg/plugins/approve/approvers/owners_test.go

@@ -42,7 +42,7 @@ func (f FakeRepo) MinimumReviewersForFile(path string) int {
 	if out, ok := f.minReviewersMap[path]; ok {
 		return out
 	}
-	return 0
+	return 1
 }
 
 func (f FakeRepo) Approvers(path string) sets.String {