fix(prow/plugins/approve): support get approvers when used full config

prow/plugins/approve/approve_test.go

@@ -133,10 +133,28 @@ func (fr fakeRepo) Filenames() ownersconfig.Filenames {
 }
 
 func (fr fakeRepo) Approvers(path string) layeredsets.String {
-	return fr.approvers[path]
+	ret := fr.approvers[path]
+	if ret.Len() > 0 || path == "" {
+		return ret
+	}
+
+	p := filepath.Dir(path)
+	if p == "." {
+		p = ""
+	}
+	return fr.Approvers(p)
 }
 func (fr fakeRepo) LeafApprovers(path string) sets.String {
-	return fr.leafApprovers[path]
+	ret := fr.leafApprovers[path]
+	if ret.Len() > 0 || path == "" {
+		return ret
+	}
+
+	p := filepath.Dir(path)
+	if p == "." {
+		p = ""
+	}
+	return fr.LeafApprovers(p)
 }
 func (fr fakeRepo) FindApproverOwnersForFile(path string) string {
 	return fr.approverOwners[path]

prow/plugins/approve/approvers/approvers_test.go

@@ -158,7 +158,7 @@ func TestGetFiles(t *testing.T) {
 			testName:          "Single File PR in B No One Approved",
 			filenames:         []string{"b/test.go"},
 			currentlyApproved: sets.NewString(),
-			expectedFiles:     []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, "master"}},
+			expectedFiles:     []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, nil, "master"}},
 		},
 		{
 			testName:          "Single File PR in B Fully Approved",
@@ -170,15 +170,15 @@ func TestGetFiles(t *testing.T) {
 			testName:          "Single Root File PR No One Approved",
 			filenames:         []string{"kubernetes.go"},
 			currentlyApproved: sets.NewString(),
-			expectedFiles:     []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, "master"}},
+			expectedFiles:     []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, nil, "master"}},
 		},
 		{
 			testName:          "Combo and Other; Neither Approved",
 			filenames:         []string{"a/combo/test.go", "a/d/test.go"},
 			currentlyApproved: sets.NewString(),
 			expectedFiles: []File{
-				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, "master"},
-				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
+				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, nil, "master"},
+				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
 			},
 		},
 		{
@@ -187,7 +187,7 @@ func TestGetFiles(t *testing.T) {
 			currentlyApproved: eApprovers,
 			expectedFiles: []File{
 				ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, eApprovers, "master"},
-				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
+				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
 			},
 		},
 		{
@@ -205,7 +205,7 @@ func TestGetFiles(t *testing.T) {
 			currentlyApproved: cApprovers,
 			expectedFiles: []File{
 				ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
-				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
+				UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
 				ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "c", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
 			},
 		},

prow/plugins/approve/approvers/owners.go

@@ -73,9 +73,20 @@ func NewOwners(log *logrus.Entry, filenames []string, r Repo, s int64) Owners {
 // GetApprovers returns a map from ownersFiles -> people that are approvers in them
 func (o Owners) GetApprovers() map[string]sets.String {
 	ownersToApprovers := map[string]sets.String{}
+	for _, toApprove := range o.filenames {
+		ownersFile := o.repo.FindApproverOwnersForFile(toApprove)
+		approvers := o.repo.Approvers(toApprove).Set()
+		if _, ok := ownersToApprovers[ownersFile]; !ok {
+			ownersToApprovers[ownersFile] = sets.NewString()
+		}
+		ownersToApprovers[ownersFile] = ownersToApprovers[ownersFile].Union(approvers)
+	}
 
-	for ownersFilename := range o.GetOwnersSet() {
-		ownersToApprovers[ownersFilename] = o.repo.Approvers(ownersFilename).Set()
+	owners := o.GetOwnersSet()
+	for k := range ownersToApprovers {
+		if !owners.Has(k) {
+			delete(ownersToApprovers, k)
+		}
 	}
 
 	return ownersToApprovers
@@ -85,8 +96,20 @@ func (o Owners) GetApprovers() map[string]sets.String {
 func (o Owners) GetLeafApprovers() map[string]sets.String {
 	ownersToApprovers := map[string]sets.String{}
 
-	for fn := range o.GetOwnersSet() {
-		ownersToApprovers[fn] = o.repo.LeafApprovers(fn)
+	for _, toApprove := range o.filenames {
+		ownersFile := o.repo.FindApproverOwnersForFile(toApprove)
+		approvers := o.repo.LeafApprovers(toApprove)
+		if _, ok := ownersToApprovers[ownersFile]; !ok {
+			ownersToApprovers[ownersFile] = sets.NewString()
+		}
+		ownersToApprovers[ownersFile] = ownersToApprovers[ownersFile].Union(approvers)
+	}
+
+	owners := o.GetOwnersSet()
+	for k := range ownersToApprovers {
+		if !owners.Has(k) {
+			delete(ownersToApprovers, k)
+		}
 	}
 
 	return ownersToApprovers
@@ -122,16 +145,19 @@ func (o Owners) GetReverseMap(approvers map[string]sets.String) map[string]sets.
 	return approverOwnersfiles
 }
 
-func findMostCoveringApprover(allApprovers []string, reverseMap map[string]sets.String, unapproved sets.String) string {
+func findMostCoveringApprover(allApprovers []string, coveredApproversSet sets.String, reverseMap map[string]sets.String, unapproved sets.String) string {
 	maxCovered := 0
 	var bestPerson string
 	for _, approver := range allApprovers {
 		filesCanApprove := reverseMap[approver]
-		if filesCanApprove.Intersection(unapproved).Len() > maxCovered {
+		if filesCanApprove.Intersection(unapproved).Len() > maxCovered && !coveredApproversSet.Has(approver) {
 			maxCovered = len(filesCanApprove)
 			bestPerson = approver
 		}
 	}
+
+	// todo: make it better.
+
 	return bestPerson
 }
 
@@ -169,7 +195,7 @@ func (o Owners) KeepCoveringApprovers(reverseMap map[string]sets.String, knownAp
 func (o Owners) GetSuggestedApprovers(reverseMap map[string]sets.String, potentialApprovers []string) sets.String {
 	ap := NewApprovers(o)
 	for !ap.RequirementsMet() {
-		newApprover := findMostCoveringApprover(potentialApprovers, reverseMap, ap.UnapprovedFiles())
+		newApprover := findMostCoveringApprover(potentialApprovers, ap.GetCurrentApproversSet(), reverseMap, ap.UnapprovedFiles())
 		if newApprover == "" {
 			o.log.Debugf("Couldn't find/suggest approvers for each files. Unapproved: %q", ap.UnapprovedFiles().List())
 			return ap.GetCurrentApproversSet()
@@ -441,9 +467,17 @@ func (ap Approvers) NoIssueApprovers() map[string]Approval {
 // UnapprovedFiles returns owners files that still need approval
 func (ap Approvers) UnapprovedFiles() sets.String {
 	unapproved := sets.NewString()
-	for fn, approvers := range ap.GetFilesApprovers() {
-		if len(approvers) == 0 {
-			unapproved.Insert(fn)
+	ownersSet := ap.owners.GetOwnersSet()
+	currentApprovers := ap.GetCurrentApproversSetCased()
+
+	for _, toApprove := range ap.owners.filenames {
+		ownersFile := ap.owners.repo.FindApproverOwnersForFile(toApprove)
+		if !ownersSet.Has(ownersFile) {
+			continue
+		}
+
+		if CaseInsensitiveIntersection(ap.owners.repo.Approvers(toApprove).Set(), currentApprovers).Len() == 0 {
+			unapproved.Insert(ownersFile)
 		}
 	}
 	return unapproved
@@ -453,12 +487,14 @@ func (ap Approvers) UnapprovedFiles() sets.String {
 func (ap Approvers) GetFiles(baseURL *url.URL, branch string) []File {
 	var allOwnersFiles []File
 	filesApprovers := ap.GetFilesApprovers()
+	unapproverdFiles := ap.UnapprovedFiles()
 	for _, file := range ap.owners.GetOwnersSet().List() {
-		if len(filesApprovers[file]) == 0 {
+		if unapproverdFiles.Has(file) {
 			allOwnersFiles = append(allOwnersFiles, UnapprovedFile{
 				baseURL:        baseURL,
 				filepath:       file,
 				ownersFilename: ap.owners.repo.Filenames().Owners,
+				approvers:      filesApprovers[file],
 				branch:         branch,
 			})
 		} else {
@@ -582,7 +618,9 @@ type UnapprovedFile struct {
 	baseURL        *url.URL
 	filepath       string
 	ownersFilename string
-	branch         string
+	// approvers is the set of users that partially approved this file change.
+	approvers sets.String
+	branch    string
 }
 
 func (a ApprovedFile) String() string {
@@ -608,6 +646,9 @@ func (ua UnapprovedFile) String() string {
 		ua.branch,
 		fullOwnersPath,
 	)
+	if ua.approvers.Len() > 0 {
+		return fmt.Sprintf("- **[%s](%s)** [%v]\n  > Need more approvers for rest parts.\n", fullOwnersPath, link, strings.Join(ua.approvers.List(), ","))
+	}
 	return fmt.Sprintf("- **[%s](%s)**\n", fullOwnersPath, link)
 }
 

prow/plugins/approve/approvers/owners_test.go

@@ -46,11 +46,29 @@ func (f FakeRepo) Filenames() ownersconfig.Filenames {
 }
 
 func (f FakeRepo) Approvers(path string) layeredsets.String {
-	return f.approversMap[path]
+	ret := f.approversMap[path]
+	if ret.Len() > 0 || path == "" {
+		return ret
+	}
+
+	p := filepath.Dir(path)
+	if p == "." {
+		p = ""
+	}
+	return f.Approvers(p)
 }
 
 func (f FakeRepo) LeafApprovers(path string) sets.String {
-	return f.leafApproversMap[path]
+	ret := f.leafApproversMap[path]
+	if ret.Len() > 0 || path == "" {
+		return ret
+	}
+
+	p := filepath.Dir(path)
+	if p == "." {
+		p = ""
+	}
+	return f.LeafApprovers(p)
 }
 
 func (f FakeRepo) FindApproverOwnersForFile(path string) string {
@@ -548,7 +566,7 @@ func TestFindMostCoveringApprover(t *testing.T) {
 			seed:      TestSeed,
 			log:       logrus.WithField("plugin", "some_plugin"),
 		}
-		bestPerson := findMostCoveringApprover(testOwners.GetAllPotentialApprovers(), testOwners.GetReverseMap(testOwners.GetLeafApprovers()), test.unapproved)
+		bestPerson := findMostCoveringApprover(testOwners.GetAllPotentialApprovers(), nil, testOwners.GetReverseMap(testOwners.GetLeafApprovers()), test.unapproved)
 		if test.expectedMostCovering.Intersection(sets.NewString(bestPerson)).Len() != 1 {
 			t.Errorf("Failed for test %v.  Didn't correct approvers list.  Expected: %v. Found %v", test.testName, test.expectedMostCovering, bestPerson)
 		}