From ed3e68d6c419391604f257f130be58f96b67aa61 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Thu, 26 Dec 2024 13:34:36 +0000
Subject: [PATCH 01/20] feat: minimum required reviewers on OWNERS

---
 pkg/plugins/approve/approve_test.go |  4 +++
 pkg/plugins/lgtm/lgtm_test.go       | 19 +++++-----
 pkg/repoowners/repoowners.go        | 56 +++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index cf1e0895f..1ca316f55 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -1306,6 +1306,10 @@ func (fro fakeRepoOwners) RequiredReviewers(path string) sets.String {
 	return sets.NewString()
 }
 
+func (fro fakeRepoOwners) MinimumAmountOfRequiredReviewers(path string) int {
+	return 1
+}
+
 func TestHandleGenericComment(t *testing.T) {
 	tests := []struct {
 		name              string
diff --git a/pkg/plugins/lgtm/lgtm_test.go b/pkg/plugins/lgtm/lgtm_test.go
index c36c99e50..00d4dbfc0 100644
--- a/pkg/plugins/lgtm/lgtm_test.go
+++ b/pkg/plugins/lgtm/lgtm_test.go
@@ -68,15 +68,16 @@ func (fp *fakePruner) PruneComments(pr bool, shouldPrune func(*scm.Comment) bool
 
 var _ repoowners.RepoOwner = &fakeRepoOwners{}
 
-func (f *fakeRepoOwners) FindApproverOwnersForFile(path string) string  { return "" }
-func (f *fakeRepoOwners) FindReviewersOwnersForFile(path string) string { return "" }
-func (f *fakeRepoOwners) FindLabelsForFile(path string) sets.String     { return nil }
-func (f *fakeRepoOwners) IsNoParentOwners(path string) bool             { return false }
-func (f *fakeRepoOwners) LeafApprovers(path string) sets.String         { return nil }
-func (f *fakeRepoOwners) Approvers(path string) sets.String             { return f.approvers[path] }
-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) FindApproverOwnersForFile(path string) string     { return "" }
+func (f *fakeRepoOwners) FindReviewersOwnersForFile(path string) string    { return "" }
+func (f *fakeRepoOwners) FindLabelsForFile(path string) sets.String        { return nil }
+func (f *fakeRepoOwners) IsNoParentOwners(path string) bool                { return false }
+func (f *fakeRepoOwners) LeafApprovers(path string) sets.String            { return nil }
+func (f *fakeRepoOwners) Approvers(path string) sets.String                { return f.approvers[path] }
+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) MinimumAmountOfRequiredReviewers(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..e44f5e0f0 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
+	MinimumAmountOfRequiredReviewers(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
+	minReviewers      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),
+		minReviewers:      make(map[string]map[*regexp.Regexp]int),
 		options:           make(map[string]dirOptions),
 
 		dirExcludes: dirExcludes,
@@ -554,6 +558,16 @@ func (o *RepoOwners) applyConfigToPath(path string, re *regexp.Regexp, config *C
 		}
 		o.labels[path][re] = sets.NewString(config.Labels...)
 	}
+
+	if o.minReviewers[path] == nil {
+		o.minReviewers[path] = make(map[*regexp.Regexp]int)
+	}
+	if config.MinimumReviewers != nil {
+		o.minReviewers[path][re] = *config.MinimumReviewers
+	} else {
+		// Default to 1 min reviewer if not specified
+		o.minReviewers[path][re] = 1
+	}
 }
 
 func (o *RepoOwners) applyOptionsToPath(path string, opts dirOptions) {
@@ -671,6 +685,41 @@ func (o *RepoOwners) entriesForFile(path string, people map[string]map[*regexp.R
 	return out
 }
 
+func (o *RepoOwners) minimumReviewersForFile(path string, minRequiredReviewers map[string]map[*regexp.Regexp]int) 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 minRequiredReviewers[d] {
+			if re == nil || re.MatchString(relative) {
+				foundMinReviewer = &s
+				break
+			}
+		}
+		if d == baseDirConvention {
+			break
+		}
+		d = filepath.Dir(d)
+		d = canonicalize(d)
+	}
+	if foundMinReviewer == nil {
+		// If we didn't find a minimum reviewer count, default to 1
+		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
@@ -708,3 +757,10 @@ func (o *RepoOwners) Reviewers(path string) sets.String {
 func (o *RepoOwners) RequiredReviewers(path string) sets.String {
 	return o.entriesForFile(path, o.requiredReviewers, false)
 }
+
+// MinimumAmountOfRequiredReviewers returns the minimum number of reviewers required to approve 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 1 is returned as a default.
+func (o *RepoOwners) MinimumAmountOfRequiredReviewers(path string) int {
+	return o.minimumReviewersForFile(path, o.minReviewers)
+}

From f7287f47550acd1ab62387398c98bdf3ce890e27 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 27 Dec 2024 22:28:21 +0000
Subject: [PATCH 02/20] test: update for minRequiredReviewers

---
 pkg/repoowners/repoowners_test.go | 69 ++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 5 deletions(-)

diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index 4969f8578..569752e55 100644
--- a/pkg/repoowners/repoowners_test.go
+++ b/pkg/repoowners/repoowners_test.go
@@ -58,6 +58,7 @@ reviewers:
 - jakub
 required_reviewers:
 - ben
+minimum_reviewers: 2
 labels:
 - src-code`),
 		"src/dir/conformance/OWNERS": []byte(`options:
@@ -331,6 +332,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 +356,12 @@ func TestLoadRepoOwners(t *testing.T) {
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -381,6 +389,12 @@ func TestLoadRepoOwners(t *testing.T) {
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -411,6 +425,13 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir":      patternAll("src-code"),
 				"docs/file.md": patternAll("docs"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+				"docs/file.md":        {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -444,6 +465,13 @@ func TestLoadRepoOwners(t *testing.T) {
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+				"src/doc":             {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -476,6 +504,12 @@ func TestLoadRepoOwners(t *testing.T) {
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -503,6 +537,12 @@ func TestLoadRepoOwners(t *testing.T) {
 				"":        patternAll("EVERYTHING"),
 				"src/dir": patternAll("src-code"),
 			},
+			expectedMinRequiredReviewers: map[string]map[string]int{
+				"":                    {"": 1},
+				"src":                 {"": 1},
+				"src/dir":             {"": 2},
+				"src/dir/conformance": {"": 1},
+			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
 					NoParentOwners: true,
@@ -544,7 +584,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 +600,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.minReviewers)
 		if !reflect.DeepEqual(test.expectedOptions, ro.options) {
 			t.Errorf("Expected options to be:\n%#v\ngot:\n%#v.", test.expectedOptions, ro.options)
 		}

From e17522556672a534e0b672a64b5223c2c06ba8aa Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sat, 28 Dec 2024 13:27:41 +0000
Subject: [PATCH 03/20] test: add tests for min required approvers

---
 pkg/plugins/approve/approve_test.go           |  3 +
 .../approve/approvers/approvers_test.go       | 88 +++++++++++++++----
 pkg/plugins/approve/approvers/owners.go       | 26 +++++-
 pkg/plugins/approve/approvers/owners_test.go  | 14 +++
 4 files changed, 114 insertions(+), 17 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index 1ca316f55..b6da6b471 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -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]
 }
diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index 400cfeec2..d6382d1e2 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"
@@ -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)
+		})
 	}
 }
 
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 4511edef5..87e3490a4 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
+	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.
+// 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:
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index 24abcc4b3..5a4e320fe 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) 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() {

From b6f747e36de41ab02ddeca4a141d507cdf8239de Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 12:55:24 +0000
Subject: [PATCH 04/20] feat: add required approvers text to message

---
 pkg/plugins/approve/approvers/owners.go | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 87e3490a4..04c45381d 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -456,6 +456,10 @@ func (ap Approvers) UnapprovedFiles() sets.String {
 	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{}
@@ -659,6 +663,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}}  

From 4fc16e28e788a294aebf17155681b04a43c6174c Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 12:55:40 +0000
Subject: [PATCH 05/20] test: add updated text to tests

---
 pkg/plugins/approve/approve_test.go | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index b6da6b471..e8dcd0828 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -248,6 +248,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -613,6 +614,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).
 
@@ -678,6 +680,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 				newTestComment("k8s-ci-robot", `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -756,6 +759,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -826,6 +830,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -864,6 +869,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -940,6 +946,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -975,6 +982,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 

From d96925a84dcfccab413a5214d863f1dd8a7848d4 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 13:02:53 +0000
Subject: [PATCH 06/20] refactor: simplify AreFilesApproved

---
 pkg/plugins/approve/approvers/owners.go | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 04c45381d..e23bc74a9 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -525,9 +525,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 {
-	currentApproversCount := ap.GetCurrentApproversSet().Len()
-	requiredApproversCount := ap.owners.GetRequiredApproversCount()
-	return ap.UnapprovedFiles().Len() == 0 && currentApproversCount >= requiredApproversCount
+	return ap.UnapprovedFiles().Len() == 0 && ap.GetRemainingRequiredApprovers() <= 0
 }
 
 // RequirementsMet returns a bool indicating whether the PR has met all approval requirements:

From 8473da999e817c67607dc16fb95f1289fab63224 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 14:58:53 +0000
Subject: [PATCH 07/20] fix: use the closest file to the change to get
 MinReviewers

---
 pkg/repoowners/repoowners.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pkg/repoowners/repoowners.go b/pkg/repoowners/repoowners.go
index e44f5e0f0..a4064ea8b 100644
--- a/pkg/repoowners/repoowners.go
+++ b/pkg/repoowners/repoowners.go
@@ -707,7 +707,7 @@ func (o *RepoOwners) minimumReviewersForFile(path string, minRequiredReviewers m
 				break
 			}
 		}
-		if d == baseDirConvention {
+		if foundMinReviewer != nil {
 			break
 		}
 		d = filepath.Dir(d)

From 65062ce718c3b74733386d7aa8736e77f41eacdb Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 14:59:09 +0000
Subject: [PATCH 08/20] test: add tests cases for MinReviewers

---
 pkg/repoowners/repoowners_test.go | 59 +++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index 569752e55..04d235e82 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"
@@ -917,3 +918,61 @@ func TestExpandAliases(t *testing.T) {
 		}
 	}
 }
+
+func TestGetRequiredApproversCount(t *testing.T) {
+	const (
+		baseDir   = ""
+		secondDir = "a"
+		thirdDir  = "a/b"
+	)
+
+	ro := &RepoOwners{
+		minReviewers: map[string]map[*regexp.Regexp]int{
+			baseDir:   {nil: 1},
+			secondDir: {nil: 2},
+			thirdDir:  {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: ro.minReviewers[baseDir][nil],
+		},
+		{
+			name:                      "Modified Second Dir",
+			filePath:                  filepath.Join(secondDir, "main.go"),
+			expectedRequiredApprovers: ro.minReviewers[secondDir][nil],
+		},
+		{
+			name:                      "Modified Third Dir",
+			filePath:                  filepath.Join(thirdDir, "main.go"),
+			expectedRequiredApprovers: ro.minReviewers[thirdDir][nil],
+		},
+		{
+			name:                      "Modified Nested Dir Without OWNERS (default to Third Dir)",
+			filePath:                  filepath.Join(thirdDir, "c", "main.go"),
+			expectedRequiredApprovers: ro.minReviewers[thirdDir][nil],
+		},
+		{
+			name:                      "Modified Nonexistent Dir (default to Base Dir)",
+			filePath:                  filepath.Join("nonexistent", "main.go"),
+			expectedRequiredApprovers: ro.minReviewers[baseDir][nil],
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := ro.MinimumAmountOfRequiredReviewers(tc.filePath)
+			assert.Equal(t, tc.expectedRequiredApprovers, actual)
+		})
+	}
+}

From e1459fd8a0f858e4c3150ec2b854f2d1609b10be Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Fri, 3 Jan 2025 16:04:12 +0000
Subject: [PATCH 09/20] fix: use the changed files to get the min required
 reviewers

---
 pkg/plugins/approve/approvers/approvers_test.go | 4 ++--
 pkg/plugins/approve/approvers/owners.go         | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index d6382d1e2..5db5b56ff 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -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
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index e23bc74a9..b82b43674 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -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:

From 1928d4538957b0d80000af0843d7ee166bda572a Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 13:27:21 +0000
Subject: [PATCH 10/20] feat: only set min reviewers if found in config

---
 pkg/repoowners/repoowners.go      | 17 +++++-------
 pkg/repoowners/repoowners_test.go | 46 +++++++++----------------------
 2 files changed, 20 insertions(+), 43 deletions(-)

diff --git a/pkg/repoowners/repoowners.go b/pkg/repoowners/repoowners.go
index a4064ea8b..29321aad4 100644
--- a/pkg/repoowners/repoowners.go
+++ b/pkg/repoowners/repoowners.go
@@ -172,7 +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
-	minReviewers      map[string]map[*regexp.Regexp]int
+	minimumReviewers  map[string]map[*regexp.Regexp]int
 	options           map[string]dirOptions
 
 	baseDir      string
@@ -390,7 +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),
-		minReviewers:      make(map[string]map[*regexp.Regexp]int),
+		minimumReviewers:  make(map[string]map[*regexp.Regexp]int),
 		options:           make(map[string]dirOptions),
 
 		dirExcludes: dirExcludes,
@@ -559,14 +559,11 @@ func (o *RepoOwners) applyConfigToPath(path string, re *regexp.Regexp, config *C
 		o.labels[path][re] = sets.NewString(config.Labels...)
 	}
 
-	if o.minReviewers[path] == nil {
-		o.minReviewers[path] = make(map[*regexp.Regexp]int)
-	}
 	if config.MinimumReviewers != nil {
-		o.minReviewers[path][re] = *config.MinimumReviewers
-	} else {
-		// Default to 1 min reviewer if not specified
-		o.minReviewers[path][re] = 1
+		if o.minimumReviewers[path] == nil {
+			o.minimumReviewers[path] = make(map[*regexp.Regexp]int)
+		}
+		o.minimumReviewers[path][re] = *config.MinimumReviewers
 	}
 }
 
@@ -762,5 +759,5 @@ func (o *RepoOwners) RequiredReviewers(path string) sets.String {
 // 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 1 is returned as a default.
 func (o *RepoOwners) MinimumAmountOfRequiredReviewers(path string) int {
-	return o.minimumReviewersForFile(path, o.minReviewers)
+	return o.minimumReviewersForFile(path, o.minimumReviewers)
 }
diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index 04d235e82..a0155c76d 100644
--- a/pkg/repoowners/repoowners_test.go
+++ b/pkg/repoowners/repoowners_test.go
@@ -358,10 +358,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir": patternAll("src-code"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -391,10 +388,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir": patternAll("src-code"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -427,11 +421,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"docs/file.md": patternAll("docs"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
-				"docs/file.md":        {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -467,11 +457,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir": patternAll("src-code"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
-				"src/doc":             {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -506,10 +492,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir": patternAll("src-code"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -539,10 +522,7 @@ func TestLoadRepoOwners(t *testing.T) {
 				"src/dir": patternAll("src-code"),
 			},
 			expectedMinRequiredReviewers: map[string]map[string]int{
-				"":                    {"": 1},
-				"src":                 {"": 1},
-				"src/dir":             {"": 2},
-				"src/dir/conformance": {"": 1},
+				"src/dir": {"": 2},
 			},
 			expectedOptions: map[string]dirOptions{
 				"src/dir/conformance": {
@@ -623,7 +603,7 @@ func TestLoadRepoOwners(t *testing.T) {
 		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.minReviewers)
+		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)
 		}
@@ -927,7 +907,7 @@ func TestGetRequiredApproversCount(t *testing.T) {
 	)
 
 	ro := &RepoOwners{
-		minReviewers: map[string]map[*regexp.Regexp]int{
+		minimumReviewers: map[string]map[*regexp.Regexp]int{
 			baseDir:   {nil: 1},
 			secondDir: {nil: 2},
 			thirdDir:  {nil: 3},
@@ -946,27 +926,27 @@ func TestGetRequiredApproversCount(t *testing.T) {
 		{
 			name:                      "Modified Base Dir",
 			filePath:                  filepath.Join(baseDir, "main.go"),
-			expectedRequiredApprovers: ro.minReviewers[baseDir][nil],
+			expectedRequiredApprovers: ro.minimumReviewers[baseDir][nil],
 		},
 		{
 			name:                      "Modified Second Dir",
 			filePath:                  filepath.Join(secondDir, "main.go"),
-			expectedRequiredApprovers: ro.minReviewers[secondDir][nil],
+			expectedRequiredApprovers: ro.minimumReviewers[secondDir][nil],
 		},
 		{
 			name:                      "Modified Third Dir",
 			filePath:                  filepath.Join(thirdDir, "main.go"),
-			expectedRequiredApprovers: ro.minReviewers[thirdDir][nil],
+			expectedRequiredApprovers: ro.minimumReviewers[thirdDir][nil],
 		},
 		{
 			name:                      "Modified Nested Dir Without OWNERS (default to Third Dir)",
 			filePath:                  filepath.Join(thirdDir, "c", "main.go"),
-			expectedRequiredApprovers: ro.minReviewers[thirdDir][nil],
+			expectedRequiredApprovers: ro.minimumReviewers[thirdDir][nil],
 		},
 		{
 			name:                      "Modified Nonexistent Dir (default to Base Dir)",
 			filePath:                  filepath.Join("nonexistent", "main.go"),
-			expectedRequiredApprovers: ro.minReviewers[baseDir][nil],
+			expectedRequiredApprovers: ro.minimumReviewers[baseDir][nil],
 		},
 	}
 	for _, tc := range testCases {

From 4a8664e84e3e16c3fb05a31ccf0c28b401b5a161 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 13:43:47 +0000
Subject: [PATCH 11/20] fix: escape traverse if no min reviewers found

---
 pkg/plugins/approve/approve_test.go          |  4 +--
 pkg/plugins/approve/approvers/owners.go      |  4 +--
 pkg/plugins/approve/approvers/owners_test.go |  2 +-
 pkg/plugins/lgtm/lgtm_test.go                | 20 +++++++--------
 pkg/repoowners/repoowners.go                 | 24 ++++++++---------
 pkg/repoowners/repoowners_test.go            | 27 ++++++++++++--------
 6 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index e8dcd0828..c2d95e9f7 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -149,7 +149,7 @@ type fakeRepo struct {
 	approverOwners           map[string]string
 }
 
-func (fr fakeRepo) MinimumAmountOfRequiredReviewers(path string) int {
+func (fr fakeRepo) MinimumReviewersForFile(path string) int {
 	return 1
 }
 func (fr fakeRepo) Approvers(path string) sets.String {
@@ -1317,7 +1317,7 @@ func (fro fakeRepoOwners) RequiredReviewers(path string) sets.String {
 	return sets.NewString()
 }
 
-func (fro fakeRepoOwners) MinimumAmountOfRequiredReviewers(path string) int {
+func (fro fakeRepoOwners) MinimumReviewersForFile(path string) int {
 	return 1
 }
 
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index b82b43674..1409cac0a 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -45,7 +45,7 @@ type Repo interface {
 	LeafApprovers(path string) sets.String
 	FindApproverOwnersForFile(file string) string
 	IsNoParentOwners(path string) bool
-	MinimumAmountOfRequiredReviewers(path string) int
+	MinimumReviewersForFile(path string) int
 }
 
 // Owners provides functionality related to owners of a specific code change.
@@ -79,7 +79,7 @@ func (o Owners) GetApprovers() map[string]sets.String {
 func (o Owners) GetRequiredApproversCount() int {
 	requiredApprovers := 1
 	for _, fn := range o.filenames {
-		if count := o.repo.MinimumAmountOfRequiredReviewers(fn); count > requiredApprovers {
+		if count := o.repo.MinimumReviewersForFile(fn); count > requiredApprovers {
 			requiredApprovers = count
 		}
 	}
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index 5a4e320fe..8310571ba 100644
--- a/pkg/plugins/approve/approvers/owners_test.go
+++ b/pkg/plugins/approve/approvers/owners_test.go
@@ -38,7 +38,7 @@ type FakeRepo struct {
 	minReviewersMap   map[string]int
 }
 
-func (f FakeRepo) MinimumAmountOfRequiredReviewers(path string) int {
+func (f FakeRepo) MinimumReviewersForFile(path string) int {
 	if out, ok := f.minReviewersMap[path]; ok {
 		return out
 	}
diff --git a/pkg/plugins/lgtm/lgtm_test.go b/pkg/plugins/lgtm/lgtm_test.go
index 00d4dbfc0..43feeb9b5 100644
--- a/pkg/plugins/lgtm/lgtm_test.go
+++ b/pkg/plugins/lgtm/lgtm_test.go
@@ -68,16 +68,16 @@ func (fp *fakePruner) PruneComments(pr bool, shouldPrune func(*scm.Comment) bool
 
 var _ repoowners.RepoOwner = &fakeRepoOwners{}
 
-func (f *fakeRepoOwners) FindApproverOwnersForFile(path string) string     { return "" }
-func (f *fakeRepoOwners) FindReviewersOwnersForFile(path string) string    { return "" }
-func (f *fakeRepoOwners) FindLabelsForFile(path string) sets.String        { return nil }
-func (f *fakeRepoOwners) IsNoParentOwners(path string) bool                { return false }
-func (f *fakeRepoOwners) LeafApprovers(path string) sets.String            { return nil }
-func (f *fakeRepoOwners) Approvers(path string) sets.String                { return f.approvers[path] }
-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) MinimumAmountOfRequiredReviewers(path string) int { return 1 }
+func (f *fakeRepoOwners) FindApproverOwnersForFile(path string) string  { return "" }
+func (f *fakeRepoOwners) FindReviewersOwnersForFile(path string) string { return "" }
+func (f *fakeRepoOwners) FindLabelsForFile(path string) sets.String     { return nil }
+func (f *fakeRepoOwners) IsNoParentOwners(path string) bool             { return false }
+func (f *fakeRepoOwners) LeafApprovers(path string) sets.String         { return nil }
+func (f *fakeRepoOwners) Approvers(path string) sets.String             { return f.approvers[path] }
+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 29321aad4..33a6f5d50 100644
--- a/pkg/repoowners/repoowners.go
+++ b/pkg/repoowners/repoowners.go
@@ -159,7 +159,7 @@ type RepoOwner interface {
 	LeafReviewers(path string) sets.String
 	Reviewers(path string) sets.String
 	RequiredReviewers(path string) sets.String
-	MinimumAmountOfRequiredReviewers(path string) int
+	MinimumReviewersForFile(path string) int
 }
 
 var _ RepoOwner = &RepoOwners{}
@@ -682,7 +682,11 @@ func (o *RepoOwners) entriesForFile(path string, people map[string]map[*regexp.R
 	return out
 }
 
-func (o *RepoOwners) minimumReviewersForFile(path string, minRequiredReviewers map[string]map[*regexp.Regexp]int) int {
+// 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
@@ -698,21 +702,22 @@ func (o *RepoOwners) minimumReviewersForFile(path string, minRequiredReviewers m
 			foundMinReviewer = nil
 			break
 		}
-		for re, s := range minRequiredReviewers[d] {
+		for re, s := range o.minimumReviewers[d] {
 			if re == nil || re.MatchString(relative) {
 				foundMinReviewer = &s
 				break
 			}
 		}
-		if foundMinReviewer != nil {
+		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 1
-		return 1
+		// If we didn't find a minimum reviewer count, default to 0
+		return 0
 	}
 	return *foundMinReviewer
 }
@@ -754,10 +759,3 @@ func (o *RepoOwners) Reviewers(path string) sets.String {
 func (o *RepoOwners) RequiredReviewers(path string) sets.String {
 	return o.entriesForFile(path, o.requiredReviewers, false)
 }
-
-// MinimumAmountOfRequiredReviewers returns the minimum number of reviewers required to approve 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 1 is returned as a default.
-func (o *RepoOwners) MinimumAmountOfRequiredReviewers(path string) int {
-	return o.minimumReviewersForFile(path, o.minimumReviewers)
-}
diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index a0155c76d..093debc43 100644
--- a/pkg/repoowners/repoowners_test.go
+++ b/pkg/repoowners/repoowners_test.go
@@ -901,16 +901,21 @@ func TestExpandAliases(t *testing.T) {
 
 func TestGetRequiredApproversCount(t *testing.T) {
 	const (
-		baseDir   = ""
+		// No min reviewers
+		baseDir = ""
+		// Min reviewers set to 1
 		secondDir = "a"
-		thirdDir  = "a/b"
+		// 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{
-			baseDir:   {nil: 1},
-			secondDir: {nil: 2},
-			thirdDir:  {nil: 3},
+			secondDir: {nil: 1},
+			thirdDir:  {nil: 2},
+			fourthDir: {nil: 3},
 		},
 		options: map[string]dirOptions{
 			noParentsDir: {
@@ -926,7 +931,7 @@ func TestGetRequiredApproversCount(t *testing.T) {
 		{
 			name:                      "Modified Base Dir",
 			filePath:                  filepath.Join(baseDir, "main.go"),
-			expectedRequiredApprovers: ro.minimumReviewers[baseDir][nil],
+			expectedRequiredApprovers: 0,
 		},
 		{
 			name:                      "Modified Second Dir",
@@ -939,19 +944,19 @@ func TestGetRequiredApproversCount(t *testing.T) {
 			expectedRequiredApprovers: ro.minimumReviewers[thirdDir][nil],
 		},
 		{
-			name:                      "Modified Nested Dir Without OWNERS (default to Third Dir)",
-			filePath:                  filepath.Join(thirdDir, "c", "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: ro.minimumReviewers[baseDir][nil],
+			expectedRequiredApprovers: 0,
 		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			actual := ro.MinimumAmountOfRequiredReviewers(tc.filePath)
+			actual := ro.MinimumReviewersForFile(tc.filePath)
 			assert.Equal(t, tc.expectedRequiredApprovers, actual)
 		})
 	}

From f95ca280694adc3515d355948f6f14a1377d599f Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 13:48:42 +0000
Subject: [PATCH 12/20] fix: default to 0 if no min reviewers found, in keeping
 with current flow

---
 pkg/plugins/approve/approvers/approvers_test.go | 2 +-
 pkg/plugins/approve/approvers/owners.go         | 5 +++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index 5db5b56ff..dda594d26 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -415,7 +415,7 @@ func TestIsApproved(t *testing.T) {
 			filenames:         []string{},
 			currentlyApproved: sets.NewString(),
 			testSeed:          0,
-			isApproved:        false,
+			isApproved:        true,
 		},
 		{
 			testName:          "Single Root File PR Approved",
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 1409cac0a..5c5ba18e4 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -75,9 +75,10 @@ func (o Owners) GetApprovers() map[string]sets.String {
 }
 
 // GetRequiredApproversCount returns the amount of approvers required to get a PR approved.
-// It returns the highest value for minimum required approvers across all relevant OWNERS files.
+// 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 {
-	requiredApprovers := 1
+	var requiredApprovers int
 	for _, fn := range o.filenames {
 		if count := o.repo.MinimumReviewersForFile(fn); count > requiredApprovers {
 			requiredApprovers = count

From 0cf1e3faec2521184bf2abfaffbe26fa088d54aa Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 18:39:37 +0000
Subject: [PATCH 13/20] test: add min reviewers to createFakeRepo

---
 .../approve/approvers/approvers_test.go       | 32 +++++++++----------
 pkg/plugins/approve/approvers/owners.go       |  3 +-
 pkg/plugins/approve/approvers/owners_test.go  | 29 +++++++----------
 3 files changed, 28 insertions(+), 36 deletions(-)

diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index dda594d26..d50070c59 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -106,7 +106,7 @@ func TestUnapprovedFiles(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -220,7 +220,7 @@ func TestGetFiles(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 			testApprovers.RequireIssue = false
 			for approver := range test.currentlyApproved {
 				testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -366,7 +366,7 @@ func TestGetCCs(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -526,7 +526,7 @@ func TestIsApproved(t *testing.T) {
 
 	for _, test := range tests {
 		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")})
+			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, fakeMinReviewersMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
 			for approver := range test.currentlyApproved {
 				testApprovers.AddApprover(approver, "REFERENCE", false)
 			}
@@ -662,7 +662,7 @@ func TestIsApprovedWithIssue(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = true
 		testApprovers.AssociatedIssue = test.associatedIssue
 		for approver, noissue := range test.currentlyApproved {
@@ -741,7 +741,7 @@ func TestGetFilesApprovers(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners), log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners, nil), log: logrus.WithField("plugin", "some_plugin")})
 		for _, approver := range test.approvers {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
 		}
@@ -759,7 +759,7 @@ func TestGetMessage(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -800,7 +800,7 @@ func TestGetMessageBitBucketServer(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -841,7 +841,7 @@ func TestGetMessageGitLab(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -882,7 +882,7 @@ func TestGetMessageWithPrefix(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -923,7 +923,7 @@ func TestGetMessageAllApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -965,7 +965,7 @@ func TestGetMessageNoneApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1005,7 +1005,7 @@ func TestGetMessageApprovedIssueAssociated(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1049,7 +1049,7 @@ func TestGetMessageApprovedNoIssueByPassed(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1093,7 +1093,7 @@ func TestGetMessageMDOwners(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1134,7 +1134,7 @@ func TestGetMessageDifferentGitHubLink(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}),
+			}, nil),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 5c5ba18e4..89078cd7a 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -525,8 +525,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 {
-	rr := ap.GetRemainingRequiredApprovers()
-	return ap.UnapprovedFiles().Len() == 0 && rr <= 0
+	return ap.UnapprovedFiles().Len() == 0 && ap.GetRemainingRequiredApprovers() <= 0
 }
 
 // RequirementsMet returns a bool indicating whether the PR has met all approval requirements:
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index 8310571ba..18f8fd042 100644
--- a/pkg/plugins/approve/approvers/owners_test.go
+++ b/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 1
+	return 0
 }
 
 func (f FakeRepo) Approvers(path string) sets.String {
@@ -73,7 +73,7 @@ func canonicalize(path string) string {
 	return strings.TrimSuffix(path, "/")
 }
 
-func createFakeRepo(la map[string]sets.String) FakeRepo {
+func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) FakeRepo {
 	// github doesn't use / at the root
 	a := map[string]sets.String{}
 	for dir, approvers := range la {
@@ -90,14 +90,7 @@ 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
+	return FakeRepo{approversMap: a, leafApproversMap: la, minReviewersMap: minReviewers}
 }
 
 func setToLower(s sets.String) sets.String {
@@ -122,7 +115,7 @@ func TestCreateFakeRepo(t *testing.T) {
 		"c":       cApprovers,
 		"a/combo": edcApprovers,
 	}
-	fakeRepo := createFakeRepo(FakeRepoMap)
+	fakeRepo := createFakeRepo(FakeRepoMap, nil)
 
 	tests := []struct {
 		testName              string
@@ -230,7 +223,7 @@ func TestGetLeafApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -293,7 +286,7 @@ func TestGetOwnersSet(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -369,7 +362,7 @@ func TestGetSuggestedApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -455,7 +448,7 @@ func TestGetAllPotentialApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -536,7 +529,7 @@ func TestFindMostCoveringApprover(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -597,7 +590,7 @@ func TestGetReverseMap(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -674,7 +667,7 @@ func TestGetShuffledApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap),
+			repo:      createFakeRepo(FakeRepoMap, nil),
 			seed:      test.seed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}

From a2fa46015707c08de2ddf9fc2a4cc8e3d3662b48 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 19:08:15 +0000
Subject: [PATCH 14/20] test: approve handler for minimum reviewers

---
 pkg/plugins/approve/approve_test.go | 125 ++++++++++++++++++++++++++--
 1 file changed, 116 insertions(+), 9 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index c2d95e9f7..2a360b36d 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -147,10 +147,14 @@ 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 {
-	return 1
+	if n, ok := fr.minimumReviewers[path]; ok {
+		return n
+	}
+	return 0
 }
 func (fr fakeRepo) Approvers(path string) sets.String {
 	return fr.approvers[path]
@@ -248,7 +252,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -614,7 +617,6 @@ 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).
 
@@ -680,7 +682,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 				newTestComment("k8s-ci-robot", `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -759,7 +760,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -830,7 +830,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -869,7 +868,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -946,7 +944,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -982,7 +979,6 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -1138,6 +1134,111 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 </details>
 <!-- META={"approvers":["alice"]} -->`,
 		},
+		{
+			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).
+
+<details open>
+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
+</details>
+<!-- META={"approvers":["derek"]} -->`,
+		},
+		{
+			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 
+You can assign the PR to them by writing ` + "`/assign `" + ` 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":[]} -->`,
+		}, {
+			name:     "2 minimum reviewers - 2 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:  true,
+			expectComment: true,
+			expectedComment: `[APPROVALNOTIFIER] This PR is **APPROVED**
+
+This pull-request has been approved by: *[alice](<> "Approved")*, *[derek](<> "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)
+
+<details >
+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":[]} -->`,
+		},
 	}
 
 	fr := fakeRepo{
@@ -1145,17 +1246,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"),
 		},
 		leafApprovers: map[string]sets.String{
 			"a":   sets.NewString("alice"),
 			"a/b": sets.NewString("bob"),
 			"c":   sets.NewString("cblecker", "cjwagner"),
+			"d":   sets.NewString("derek"),
 		},
 		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/d.go": 2,
 		},
 	}
 

From 0d5911c959bd83932255ba100811c7efc70b240f Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Sun, 5 Jan 2025 19:14:35 +0000
Subject: [PATCH 15/20] chore: remove redundant function

---
 pkg/plugins/approve/approvers/owners.go | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 89078cd7a..33ffa9d19 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/pkg/plugins/approve/approvers/owners.go
@@ -188,17 +188,11 @@ 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
 }
 

From c242327eed4dea055f8e36d372a7c4b9ddc2d979 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Tue, 7 Jan 2025 14:55:15 +0000
Subject: [PATCH 16/20] fix: default to 1 min reviewer if not found

---
 pkg/plugins/approve/approve_test.go | 13 +++++++++++--
 pkg/repoowners/repoowners.go        |  2 +-
 pkg/repoowners/repoowners_test.go   |  4 ++--
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index 2a360b36d..b4355b990 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -154,7 +154,7 @@ func (fr fakeRepo) MinimumReviewersForFile(path string) int {
 	if n, ok := fr.minimumReviewers[path]; ok {
 		return n
 	}
-	return 0
+	return 1
 }
 func (fr fakeRepo) Approvers(path string) sets.String {
 	return fr.approvers[path]
@@ -252,6 +252,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -617,6 +618,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).
 
@@ -682,6 +684,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 				newTestComment("k8s-ci-robot", `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -760,6 +763,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -830,6 +834,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -868,6 +873,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -944,6 +950,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -979,6 +986,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			expectedComment: `[APPROVALNOTIFIER] This PR is **NOT APPROVED**
 
 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.
 
@@ -1203,7 +1211,8 @@ Approvers can indicate their approval by writing ` + "`/approve`" + ` in a comme
 Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a comment
 </details>
 <!-- META={"approvers":[]} -->`,
-		}, {
+		},
+		{
 			name:     "2 minimum reviewers - 2 approval",
 			hasLabel: false,
 			files:    []string{"d/d.go"},
diff --git a/pkg/repoowners/repoowners.go b/pkg/repoowners/repoowners.go
index 33a6f5d50..728a7d581 100644
--- a/pkg/repoowners/repoowners.go
+++ b/pkg/repoowners/repoowners.go
@@ -717,7 +717,7 @@ func (o *RepoOwners) MinimumReviewersForFile(path string) int {
 	}
 	if foundMinReviewer == nil {
 		// If we didn't find a minimum reviewer count, default to 0
-		return 0
+		return 1
 	}
 	return *foundMinReviewer
 }
diff --git a/pkg/repoowners/repoowners_test.go b/pkg/repoowners/repoowners_test.go
index 093debc43..2bc583b44 100644
--- a/pkg/repoowners/repoowners_test.go
+++ b/pkg/repoowners/repoowners_test.go
@@ -931,7 +931,7 @@ func TestGetRequiredApproversCount(t *testing.T) {
 		{
 			name:                      "Modified Base Dir",
 			filePath:                  filepath.Join(baseDir, "main.go"),
-			expectedRequiredApprovers: 0,
+			expectedRequiredApprovers: 1,
 		},
 		{
 			name:                      "Modified Second Dir",
@@ -951,7 +951,7 @@ func TestGetRequiredApproversCount(t *testing.T) {
 		{
 			name:                      "Modified Nonexistent Dir (default to Base Dir)",
 			filePath:                  filepath.Join("nonexistent", "main.go"),
-			expectedRequiredApprovers: 0,
+			expectedRequiredApprovers: 1,
 		},
 	}
 	for _, tc := range testCases {

From 95470e090cc4bedea1589354dab3d7e5ebbee8f2 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Wed, 15 Jan 2025 19:05:23 +0000
Subject: [PATCH 17/20] fix: handle case where multiple reviewers but not all
 owners

---
 pkg/plugins/approve/approve_test.go           | 77 +++++++++++++------
 .../approve/approvers/approvers_test.go       | 20 ++++-
 pkg/plugins/approve/approvers/owners.go       |  8 +-
 pkg/plugins/approve/approvers/owners_test.go  |  2 +-
 4 files changed, 74 insertions(+), 33 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index b4355b990..2c7ab75a7 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"
@@ -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,7 +1212,7 @@ 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",
@@ -1218,7 +1220,7 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			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,13 +1243,49 @@ 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{
@@ -1255,13 +1293,13 @@ 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"),
+			"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")
 			}
 		})
 	}
diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index d50070c59..829c747fd 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/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)
 			}
diff --git a/pkg/plugins/approve/approvers/owners.go b/pkg/plugins/approve/approvers/owners.go
index 33ffa9d19..2db2af326 100644
--- a/pkg/plugins/approve/approvers/owners.go
+++ b/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:
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index 18f8fd042..fe055a404 100644
--- a/pkg/plugins/approve/approvers/owners_test.go
+++ b/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 {

From 57c16b16bb606cb293db92fe76632d919f97762b Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Wed, 15 Jan 2025 21:22:15 +0000
Subject: [PATCH 18/20] fix: text rendering in tests

---
 pkg/plugins/approve/approve_test.go | 40 ++++++++++++++---------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/pkg/plugins/approve/approve_test.go b/pkg/plugins/approve/approve_test.go
index 2c7ab75a7..88de22a7c 100644
--- a/pkg/plugins/approve/approve_test.go
+++ b/pkg/plugins/approve/approve_test.go
@@ -253,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -685,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -764,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -835,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -874,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -951,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -987,8 +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:  
-The changes made require 1 more approval(s).
+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.
 
@@ -1162,8 +1162,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			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**
+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).
@@ -1198,8 +1198,8 @@ Approvers can cancel approval by writing ` + "`/approve cancel`" + ` in a commen
 			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**
+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).
@@ -1270,8 +1270,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: *[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**
+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).

From ef540993044c3478968787660817c87fadc99572 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Thu, 16 Jan 2025 13:33:04 +0000
Subject: [PATCH 19/20] test: add case for 2 required approvals from different
 OWNERS

---
 pkg/plugins/approve/approvers/approvers_test.go | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index 829c747fd..3be1bdc49 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -400,11 +400,9 @@ func TestIsApproved(t *testing.T) {
 		"minReviewers/2Required": minApprovers2Required,
 	}
 	fakeMinReviewersMap := map[string]int{
-		"minReviewers":           1,
-		"minReviewers/2Required": 2,
+		"minReviewers/2RequiredNoParents": 2,
 	}
 	fakeNoParentsOwnersMap := map[string]bool{
-		"minReviewers":           true,
 		"minReviewers/2Required": true,
 	}
 
@@ -534,6 +532,13 @@ func TestIsApproved(t *testing.T) {
 			currentlyApproved: sets.NewString("Alice", "Derek"),
 			isApproved:        false,
 		},
+		{
+			testName:          "Min Reviewers/2required & other OWNERS; One from each OWNERS",
+			filenames:         []string{"a/test.go", "minReviewers/2Required/test.go"},
+			testSeed:          0,
+			currentlyApproved: sets.NewString("Alice", "Anne"),
+			isApproved:        true,
+		},
 	}
 
 	for _, test := range tests {

From d55358e81f867382bfa2903c980ecc1219feedc1 Mon Sep 17 00:00:00 2001
From: James Skeoch <jaa.skeoch@gmail.com>
Date: Mon, 20 Jan 2025 18:53:02 +0000
Subject: [PATCH 20/20] test: add more coverage of minReviewers tests

---
 .../approve/approvers/approvers_test.go       | 348 +++++++++++++++---
 pkg/plugins/approve/approvers/owners_test.go  |  20 +-
 2 files changed, 303 insertions(+), 65 deletions(-)

diff --git a/pkg/plugins/approve/approvers/approvers_test.go b/pkg/plugins/approve/approvers/approvers_test.go
index 3be1bdc49..98544a54a 100644
--- a/pkg/plugins/approve/approvers/approvers_test.go
+++ b/pkg/plugins/approve/approvers/approvers_test.go
@@ -106,7 +106,7 @@ func TestUnapprovedFiles(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -220,7 +220,7 @@ func TestGetFiles(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.testName, func(t *testing.T) {
-			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
+			testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: TestSeed, log: logrus.WithField("plugin", "some_plugin")})
 			testApprovers.RequireIssue = false
 			for approver := range test.currentlyApproved {
 				testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -366,7 +366,7 @@ func TestGetCCs(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: test.testSeed, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = false
 		for approver := range test.currentlyApproved {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
@@ -387,23 +387,13 @@ 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,
-		"minReviewers":           minApproversRoot,
-		"minReviewers/2Required": minApprovers2Required,
-	}
-	fakeMinReviewersMap := map[string]int{
-		"minReviewers/2RequiredNoParents": 2,
-	}
-	fakeNoParentsOwnersMap := map[string]bool{
-		"minReviewers/2Required": true,
+		"":        rootApprovers,
+		"a":       aApprovers,
+		"b":       bApprovers,
+		"c":       cApprovers,
+		"a/d":     dApprovers,
+		"a/combo": edcApprovers,
 	}
 
 	tests := []struct {
@@ -483,74 +473,322 @@ func TestIsApproved(t *testing.T) {
 			currentlyApproved: sets.NewString("Anne"),
 			isApproved:        false,
 		},
+	}
+
+	for _, test := range tests {
+		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:          "Min Reviewers Root; 1 approval",
-			filenames:         []string{"minReviewers/test.go"},
+			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:          "Min Reviewers/2required; 1 approval",
-			filenames:         []string{"minReviewers/2Required/test.go"},
+			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:          "Min Reviewers/2required; 2 approvals",
-			filenames:         []string{"minReviewers/2Required/test.go"},
+			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:          "Min Reviewers/2required; 2 approvals but not from required approvers",
-			filenames:         []string{"minReviewers/2Required/test.go"},
+			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("Tim", "Sally"),
+			currentlyApproved: sets.NewString("Bob", "Tim"),
 			isApproved:        false,
 		},
 		{
-			testName:          "Min Reviewers/2required & root; 1 approval",
-			filenames:         []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
+			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"),
+			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:          "Min Reviewers/2required & root; 2 approval",
-			filenames:         []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
+			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"),
+			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:          "Min Reviewers/2required & root; 1 approval, 1 not registered approver",
-			filenames:         []string{"minReviewers/test.go", "minReviewers/2Required/test.go"},
+			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("Alice", "Derek"),
+			currentlyApproved: sets.NewString("Tom", "Harry", "Alice"),
 			isApproved:        false,
 		},
 		{
-			testName:          "Min Reviewers/2required & other OWNERS; One from each OWNERS",
-			filenames:         []string{"a/test.go", "minReviewers/2Required/test.go"},
+			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", "Anne"),
+			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(FakeRepoMap, fakeMinReviewersMap)
-			fakeRepo.noParentOwnersMap = fakeNoParentsOwnersMap
+			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, "Failed for test %v", test.testName)
+			assert.Equal(t, test.isApproved, calculated)
 		})
 	}
 }
@@ -681,7 +919,7 @@ func TestIsApprovedWithIssue(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap, nil), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(FakeRepoMap), seed: 0, log: logrus.WithField("plugin", "some_plugin")})
 		testApprovers.RequireIssue = true
 		testApprovers.AssociatedIssue = test.associatedIssue
 		for approver, noissue := range test.currentlyApproved {
@@ -760,7 +998,7 @@ func TestGetFilesApprovers(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners, nil), log: logrus.WithField("plugin", "some_plugin")})
+		testApprovers := NewApprovers(Owners{filenames: test.filenames, repo: createFakeRepo(test.owners), log: logrus.WithField("plugin", "some_plugin")})
 		for _, approver := range test.approvers {
 			testApprovers.AddApprover(approver, "REFERENCE", false)
 		}
@@ -778,7 +1016,7 @@ func TestGetMessage(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -819,7 +1057,7 @@ func TestGetMessageBitBucketServer(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -860,7 +1098,7 @@ func TestGetMessageGitLab(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -901,7 +1139,7 @@ func TestGetMessageWithPrefix(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -942,7 +1180,7 @@ func TestGetMessageAllApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -984,7 +1222,7 @@ func TestGetMessageNoneApproved(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1024,7 +1262,7 @@ func TestGetMessageApprovedIssueAssociated(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1068,7 +1306,7 @@ func TestGetMessageApprovedNoIssueByPassed(t *testing.T) {
 			repo: createFakeRepo(map[string]sets.String{
 				"a": sets.NewString("Alice"),
 				"b": sets.NewString("Bill"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1112,7 +1350,7 @@ func TestGetMessageMDOwners(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
@@ -1153,7 +1391,7 @@ func TestGetMessageDifferentGitHubLink(t *testing.T) {
 				"a":           sets.NewString("Alice"),
 				"b":           sets.NewString("Bill"),
 				"b/README.md": sets.NewString("Doctor"),
-			}, nil),
+			}),
 			log: logrus.WithField("plugin", "some_plugin"),
 		},
 	)
diff --git a/pkg/plugins/approve/approvers/owners_test.go b/pkg/plugins/approve/approvers/owners_test.go
index fe055a404..63a040de3 100644
--- a/pkg/plugins/approve/approvers/owners_test.go
+++ b/pkg/plugins/approve/approvers/owners_test.go
@@ -73,7 +73,7 @@ func canonicalize(path string) string {
 	return strings.TrimSuffix(path, "/")
 }
 
-func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) FakeRepo {
+func createFakeRepo(la map[string]sets.String) FakeRepo {
 	// github doesn't use / at the root
 	a := map[string]sets.String{}
 	for dir, approvers := range la {
@@ -90,7 +90,7 @@ func createFakeRepo(la map[string]sets.String, minReviewers map[string]int) Fake
 			}
 		}
 	}
-	return FakeRepo{approversMap: a, leafApproversMap: la, minReviewersMap: minReviewers}
+	return FakeRepo{approversMap: a, leafApproversMap: la}
 }
 
 func setToLower(s sets.String) sets.String {
@@ -115,7 +115,7 @@ func TestCreateFakeRepo(t *testing.T) {
 		"c":       cApprovers,
 		"a/combo": edcApprovers,
 	}
-	fakeRepo := createFakeRepo(FakeRepoMap, nil)
+	fakeRepo := createFakeRepo(FakeRepoMap)
 
 	tests := []struct {
 		testName              string
@@ -223,7 +223,7 @@ func TestGetLeafApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -286,7 +286,7 @@ func TestGetOwnersSet(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -362,7 +362,7 @@ func TestGetSuggestedApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -448,7 +448,7 @@ func TestGetAllPotentialApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -529,7 +529,7 @@ func TestFindMostCoveringApprover(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -590,7 +590,7 @@ func TestGetReverseMap(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
@@ -667,7 +667,7 @@ func TestGetShuffledApprovers(t *testing.T) {
 	for _, test := range tests {
 		testOwners := Owners{
 			filenames: test.filenames,
-			repo:      createFakeRepo(FakeRepoMap, nil),
+			repo:      createFakeRepo(FakeRepoMap),
 			seed:      test.seed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}