Skip to content

Commit 76c82f1

Browse files
wuhuizuowuhuizuo
authored
fix(prow/plugins/approve): approving with full config OWNERS controlled (#29970)
* fix(prow/plugins/approve): support get approvers when used full config Signed-off-by: wuhuizuo <[email protected]> * fix(prow/plugins/approve): fix `Approvers.UnapprovedFiles` method Signed-off-by: wuhuizuo <[email protected]> * fix(prow/plugins/approve): fix `Owners.GetSuggestedApprovers` method Signed-off-by: wuhuizuo <[email protected]> * fix(prow/repoowners): fix `RepoOwners.entriesForFile` method `NoParentOwners` should only worked when none people got. For an example: ``` # file sub/OWNERS options: no_parent_owners: true filters: "\\.yaml": approvers: [alice] ``` But only the file `func_test.go` changed in the PR, it should use the parent OWNERS file. Signed-off-by: wuhuizuo <[email protected]> --------- Signed-off-by: wuhuizuo <[email protected]>
1 parent 2b07f93 commit 76c82f1

File tree

6 files changed

+110
-24
lines changed

6 files changed

+110
-24
lines changed

‎prow/plugins/approve/approve_test.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,28 @@ func (fr fakeRepo) Filenames() ownersconfig.Filenames {
133133
}
134134

135135
func (fr fakeRepo) Approvers(path string) layeredsets.String {
136-
return fr.approvers[path]
136+
ret := fr.approvers[path]
137+
if ret.Len() > 0 || path == "" {
138+
return ret
139+
}
140+
141+
p := filepath.Dir(path)
142+
if p == "." {
143+
p = ""
144+
}
145+
return fr.Approvers(p)
137146
}
138147
func (fr fakeRepo) LeafApprovers(path string) sets.Set[string] {
139-
return fr.leafApprovers[path]
148+
ret := fr.leafApprovers[path]
149+
if ret.Len() > 0 || path == "" {
150+
return ret
151+
}
152+
153+
p := filepath.Dir(path)
154+
if p == "." {
155+
p = ""
156+
}
157+
return fr.LeafApprovers(p)
140158
}
141159
func (fr fakeRepo) FindApproverOwnersForFile(path string) string {
142160
return fr.approverOwners[path]

‎prow/plugins/approve/approvers/approvers_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ func TestGetFiles(t *testing.T) {
158158
testName: "Single File PR in B No One Approved",
159159
filenames: []string{"b/test.go"},
160160
currentlyApproved: sets.New[string](),
161-
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, "master"}},
161+
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "b", ownersconfig.DefaultOwnersFile, nil, "master"}},
162162
},
163163
{
164164
testName: "Single File PR in B Fully Approved",
@@ -170,15 +170,15 @@ func TestGetFiles(t *testing.T) {
170170
testName: "Single Root File PR No One Approved",
171171
filenames: []string{"kubernetes.go"},
172172
currentlyApproved: sets.New[string](),
173-
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, "master"}},
173+
expectedFiles: []File{UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "", ownersconfig.DefaultOwnersFile, nil, "master"}},
174174
},
175175
{
176176
testName: "Combo and Other; Neither Approved",
177177
filenames: []string{"a/combo/test.go", "a/d/test.go"},
178178
currentlyApproved: sets.New[string](),
179179
expectedFiles: []File{
180-
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, "master"},
181-
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
180+
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, nil, "master"},
181+
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
182182
},
183183
},
184184
{
@@ -187,7 +187,7 @@ func TestGetFiles(t *testing.T) {
187187
currentlyApproved: eApprovers,
188188
expectedFiles: []File{
189189
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, eApprovers, "master"},
190-
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
190+
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
191191
},
192192
},
193193
{
@@ -205,7 +205,7 @@ func TestGetFiles(t *testing.T) {
205205
currentlyApproved: cApprovers,
206206
expectedFiles: []File{
207207
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/combo", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
208-
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, "master"},
208+
UnapprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "a/d", ownersconfig.DefaultOwnersFile, nil, "master"},
209209
ApprovedFile{&url.URL{Scheme: "https", Host: "github.com", Path: "org/repo"}, "c", ownersconfig.DefaultOwnersFile, cApprovers, "master"},
210210
},
211211
},

‎prow/plugins/approve/approvers/owners.go

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,20 @@ func NewOwners(log *logrus.Entry, filenames []string, r Repo, s int64) Owners {
7373
// GetApprovers returns a map from ownersFiles -> people that are approvers in them
7474
func (o Owners) GetApprovers() map[string]sets.Set[string] {
7575
ownersToApprovers := map[string]sets.Set[string]{}
76+
for _, toApprove := range o.filenames {
77+
ownersFile := o.repo.FindApproverOwnersForFile(toApprove)
78+
approvers := o.repo.Approvers(toApprove).Set()
79+
if _, ok := ownersToApprovers[ownersFile]; !ok {
80+
ownersToApprovers[ownersFile] = sets.New[string]()
81+
}
82+
ownersToApprovers[ownersFile] = ownersToApprovers[ownersFile].Union(approvers)
83+
}
7684

77-
for ownersFilename := range o.GetOwnersSet() {
78-
ownersToApprovers[ownersFilename] = o.repo.Approvers(ownersFilename).Set()
85+
owners := o.GetOwnersSet()
86+
for k := range ownersToApprovers {
87+
if !owners.Has(k) {
88+
delete(ownersToApprovers, k)
89+
}
7990
}
8091

8192
return ownersToApprovers
@@ -85,8 +96,20 @@ func (o Owners) GetApprovers() map[string]sets.Set[string] {
8596
func (o Owners) GetLeafApprovers() map[string]sets.Set[string] {
8697
ownersToApprovers := map[string]sets.Set[string]{}
8798

88-
for fn := range o.GetOwnersSet() {
89-
ownersToApprovers[fn] = o.repo.LeafApprovers(fn)
99+
for _, toApprove := range o.filenames {
100+
ownersFile := o.repo.FindApproverOwnersForFile(toApprove)
101+
approvers := o.repo.LeafApprovers(toApprove)
102+
if _, ok := ownersToApprovers[ownersFile]; !ok {
103+
ownersToApprovers[ownersFile] = sets.New[string]()
104+
}
105+
ownersToApprovers[ownersFile] = ownersToApprovers[ownersFile].Union(approvers)
106+
}
107+
108+
owners := o.GetOwnersSet()
109+
for k := range ownersToApprovers {
110+
if !owners.Has(k) {
111+
delete(ownersToApprovers, k)
112+
}
90113
}
91114

92115
return ownersToApprovers
@@ -122,16 +145,19 @@ func (o Owners) GetReverseMap(approvers map[string]sets.Set[string]) map[string]
122145
return approverOwnersfiles
123146
}
124147

125-
func findMostCoveringApprover(allApprovers []string, reverseMap map[string]sets.Set[string], unapproved sets.Set[string]) string {
148+
func findMostCoveringApprover(allApprovers []string, coveredApproversSet sets.Set[string], reverseMap map[string]sets.Set[string], unapproved sets.Set[string]) string {
126149
maxCovered := 0
127150
var bestPerson string
128151
for _, approver := range allApprovers {
129152
filesCanApprove := reverseMap[approver]
130-
if filesCanApprove.Intersection(unapproved).Len() > maxCovered {
153+
if filesCanApprove.Intersection(unapproved).Len() > maxCovered && !coveredApproversSet.Has(approver) {
131154
maxCovered = len(filesCanApprove)
132155
bestPerson = approver
133156
}
134157
}
158+
159+
// todo: make it better.
160+
135161
return bestPerson
136162
}
137163

@@ -169,7 +195,7 @@ func (o Owners) KeepCoveringApprovers(reverseMap map[string]sets.Set[string], kn
169195
func (o Owners) GetSuggestedApprovers(reverseMap map[string]sets.Set[string], potentialApprovers []string) sets.Set[string] {
170196
ap := NewApprovers(o)
171197
for !ap.RequirementsMet() {
172-
newApprover := findMostCoveringApprover(potentialApprovers, reverseMap, ap.UnapprovedFiles())
198+
newApprover := findMostCoveringApprover(potentialApprovers, ap.GetCurrentApproversSet(), reverseMap, ap.UnapprovedFiles())
173199
if newApprover == "" {
174200
o.log.Debugf("Couldn't find/suggest approvers for each files. Unapproved: %q", sets.List(ap.UnapprovedFiles()))
175201
return ap.GetCurrentApproversSet()
@@ -441,9 +467,17 @@ func (ap Approvers) NoIssueApprovers() map[string]Approval {
441467
// UnapprovedFiles returns owners files that still need approval
442468
func (ap Approvers) UnapprovedFiles() sets.Set[string] {
443469
unapproved := sets.New[string]()
444-
for fn, approvers := range ap.GetFilesApprovers() {
445-
if len(approvers) == 0 {
446-
unapproved.Insert(fn)
470+
ownersSet := ap.owners.GetOwnersSet()
471+
currentApprovers := ap.GetCurrentApproversSetCased()
472+
473+
for _, toApprove := range ap.owners.filenames {
474+
ownersFile := ap.owners.repo.FindApproverOwnersForFile(toApprove)
475+
if !ownersSet.Has(ownersFile) {
476+
continue
477+
}
478+
479+
if CaseInsensitiveIntersection(ap.owners.repo.Approvers(toApprove).Set(), currentApprovers).Len() == 0 {
480+
unapproved.Insert(ownersFile)
447481
}
448482
}
449483
return unapproved
@@ -453,12 +487,14 @@ func (ap Approvers) UnapprovedFiles() sets.Set[string] {
453487
func (ap Approvers) GetFiles(baseURL *url.URL, branch string) []File {
454488
var allOwnersFiles []File
455489
filesApprovers := ap.GetFilesApprovers()
490+
unapproverdFiles := ap.UnapprovedFiles()
456491
for _, file := range sets.List(ap.owners.GetOwnersSet()) {
457-
if len(filesApprovers[file]) == 0 {
492+
if unapproverdFiles.Has(file) {
458493
allOwnersFiles = append(allOwnersFiles, UnapprovedFile{
459494
baseURL: baseURL,
460495
filepath: file,
461496
ownersFilename: ap.owners.repo.Filenames().Owners,
497+
approvers: filesApprovers[file],
462498
branch: branch,
463499
})
464500
} else {
@@ -582,7 +618,9 @@ type UnapprovedFile struct {
582618
baseURL *url.URL
583619
filepath string
584620
ownersFilename string
585-
branch string
621+
// approvers is the set of users that partially approved this file change.
622+
approvers sets.Set[string]
623+
branch string
586624
}
587625

588626
func (a ApprovedFile) String() string {
@@ -608,6 +646,9 @@ func (ua UnapprovedFile) String() string {
608646
ua.branch,
609647
fullOwnersPath,
610648
)
649+
if ua.approvers.Len() > 0 {
650+
return fmt.Sprintf("- **[%s](%s)** [%v]\n > Need more approvers for rest parts.\n", fullOwnersPath, link, strings.Join(sets.List(ua.approvers), ","))
651+
}
611652
return fmt.Sprintf("- **[%s](%s)**\n", fullOwnersPath, link)
612653
}
613654

‎prow/plugins/approve/approvers/owners_test.go

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,30 @@ func (f FakeRepo) Filenames() ownersconfig.Filenames {
4646
}
4747

4848
func (f FakeRepo) Approvers(path string) layeredsets.String {
49-
return f.approversMap[path]
49+
ret := f.approversMap[path]
50+
if ret.Len() > 0 || path == "" {
51+
return ret
52+
}
53+
54+
p := filepath.Dir(path)
55+
if p == "." {
56+
p = ""
57+
}
58+
return f.Approvers(p)
5059
}
5160

5261
func (f FakeRepo) LeafApprovers(path string) sets.Set[string] {
53-
return f.leafApproversMap[path]
62+
ret := f.leafApproversMap[path]
63+
64+
if ret.Len() > 0 || path == "" {
65+
return ret
66+
}
67+
68+
p := filepath.Dir(path)
69+
if p == "." {
70+
p = ""
71+
}
72+
return f.LeafApprovers(p)
5473
}
5574

5675
func (f FakeRepo) FindApproverOwnersForFile(path string) string {
@@ -548,10 +567,11 @@ func TestFindMostCoveringApprover(t *testing.T) {
548567
seed: TestSeed,
549568
log: logrus.WithField("plugin", "some_plugin"),
550569
}
551-
bestPerson := findMostCoveringApprover(testOwners.GetAllPotentialApprovers(), testOwners.GetReverseMap(testOwners.GetLeafApprovers()), test.unapproved)
570+
bestPerson := findMostCoveringApprover(testOwners.GetAllPotentialApprovers(), nil, testOwners.GetReverseMap(testOwners.GetLeafApprovers()), test.unapproved)
552571
if test.expectedMostCovering.Intersection(sets.New[string](bestPerson)).Len() != 1 {
553572
t.Errorf("Failed for test %v. Didn't correct approvers list. Expected: %v. Found %v", test.testName, test.expectedMostCovering, bestPerson)
554573
}
574+
555575
}
556576
}
557577

‎prow/repoowners/repoowners.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -839,7 +839,7 @@ func (o *RepoOwners) entriesForFile(path string, people map[string]map[*regexp.R
839839
if d == baseDirConvention {
840840
break
841841
}
842-
if o.options[d].NoParentOwners {
842+
if o.options[d].NoParentOwners && out.Len() > 0 {
843843
break
844844
}
845845
d = filepath.Dir(d)

‎prow/repoowners/repoowners_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1045,6 +1045,13 @@ func TestGetApprovers(t *testing.T) {
10451045
expectedLeafOwners: ro.approvers[noParentsFilterDir][txtFileReg],
10461046
expectedAllOwners: ro.approvers[noParentsFilterDir][txtFileReg],
10471047
},
1048+
{
1049+
name: "Modified regexp not matched file in NoParentOwners Dir Only",
1050+
filePath: filepath.Join(noParentsFilterDir, "testFile.go_to_parent"),
1051+
expectedOwnersPath: baseDir,
1052+
expectedLeafOwners: ro.approvers[baseDir][nil],
1053+
expectedAllOwners: ro.approvers[baseDir][nil],
1054+
},
10481055
{
10491056
name: "Modified Nonexistent Dir (Default to Base)",
10501057
filePath: filepath.Join(nonExistentDir, "testFile.md"),

0 commit comments

Comments
 (0)