diff --git a/prow/plugins/approve/approve_test.go b/prow/plugins/approve/approve_test.go index 21f05885b111..fdd2217c7c6f 100644 --- a/prow/plugins/approve/approve_test.go +++ b/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] diff --git a/prow/plugins/approve/approvers/approvers_test.go b/prow/plugins/approve/approvers/approvers_test.go index 66116cf0b86e..d53909f92ec9 100644 --- a/prow/plugins/approve/approvers/approvers_test.go +++ b/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"}, }, }, diff --git a/prow/plugins/approve/approvers/owners.go b/prow/plugins/approve/approvers/owners.go index 9189bad9c4c9..df7e510e9ee8 100644 --- a/prow/plugins/approve/approvers/owners.go +++ b/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) } diff --git a/prow/plugins/approve/approvers/owners_test.go b/prow/plugins/approve/approvers/owners_test.go index bbcd284d2687..f26c644ff9b3 100644 --- a/prow/plugins/approve/approvers/owners_test.go +++ b/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) }