Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement 1 for approve plugin refactoring #33

Draft
wants to merge 1 commit into
base: release
Choose a base branch
from
Draft

implement 1 for approve plugin refactoring #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions prow/plugins/approve/approvers/owners.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,26 +70,24 @@ func NewOwners(log *logrus.Entry, filenames []string, r Repo, s int64) Owners {
return Owners{filenamesUnfiltered: filenames, filenames: filenames, repo: r, seed: s, log: log}
}

// GetApprovers returns a map from ownersFiles -> people that are approvers in them
// GetApprovers returns a map from changed file -> people that are approvers in them
func (o Owners) GetApprovers() map[string]sets.String {
ownersToApprovers := map[string]sets.String{}

for ownersFilename := range o.GetOwnersSet() {
ownersToApprovers[ownersFilename] = o.repo.Approvers(ownersFilename).Set()
changedFilesToApprovers := map[string]sets.String{}
for _, f := range o.filenames {
changedFilesToApprovers[f] = o.repo.Approvers(f).Set()
}

return ownersToApprovers
return changedFilesToApprovers
}

// GetLeafApprovers returns a map from ownersFiles -> people that are approvers in them (only the leaf)
// GetLeafApprovers returns a map from changed file -> people that are approvers in them (only the leaf)
func (o Owners) GetLeafApprovers() map[string]sets.String {
ownersToApprovers := map[string]sets.String{}

for fn := range o.GetOwnersSet() {
ownersToApprovers[fn] = o.repo.LeafApprovers(fn)
changedFilesToApprovers := map[string]sets.String{}
for _, f := range o.filenames {
changedFilesToApprovers[f] = o.repo.LeafApprovers(f)
}

return ownersToApprovers
return changedFilesToApprovers
}

// GetAllPotentialApprovers returns the people from relevant owners files needed to get the PR approved
Expand All @@ -107,7 +105,7 @@ func (o Owners) GetAllPotentialApprovers() []string {
return approversOnly
}

// GetReverseMap returns a map from people -> OWNERS files for which they are an approver
// GetReverseMap returns a map from people -> changed files for which they are an approver
func (o Owners) GetReverseMap(approvers map[string]sets.String) map[string]sets.String {
approverOwnersfiles := map[string]sets.String{}
for ownersFile, approvers := range approvers {
Expand Down Expand Up @@ -401,7 +399,7 @@ func (ap Approvers) GetNoIssueApproversSet() sets.String {
func (ap Approvers) GetFilesApprovers() map[string]sets.String {
filesApprovers := map[string]sets.String{}
currentApprovers := ap.GetCurrentApproversSetCased()
for ownersFilename, potentialApprovers := range ap.owners.GetApprovers() {
for changedFilename, potentialApprovers := range ap.owners.GetApprovers() {
// The order of parameter matters here:
// - currentApprovers is the list of github handles that have approved
// - potentialApprovers is the list of handles in the OWNER
Expand All @@ -410,7 +408,7 @@ func (ap Approvers) GetFilesApprovers() map[string]sets.String {
// We want to keep the syntax of the github handle
// rather than the potential mis-cased username found in
// the OWNERS file, that's why it's the first parameter.
filesApprovers[ownersFilename] = CaseInsensitiveIntersection(currentApprovers, potentialApprovers)
filesApprovers[changedFilename] = CaseInsensitiveIntersection(currentApprovers, potentialApprovers)
}

return filesApprovers
Expand Down Expand Up @@ -449,7 +447,7 @@ func (ap Approvers) UnapprovedFiles() sets.String {
return unapproved
}

// GetFiles returns owners files that still need approval.
// GetFiles returns owners files that were approved or unapproved.
func (ap Approvers) GetFiles(baseURL *url.URL, branch string) []File {
var allOwnersFiles []File
filesApprovers := ap.GetFilesApprovers()
Expand Down
71 changes: 41 additions & 30 deletions prow/plugins/approve/approvers/owners_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,21 @@ func (f FakeRepo) Filenames() ownersconfig.Filenames {
}

func (f FakeRepo) Approvers(path string) layeredsets.String {
return f.approversMap[path]
d := filepath.Dir(path)
if d == "." {
d = ""
}

return f.approversMap[d]
}

func (f FakeRepo) LeafApprovers(path string) sets.String {
return f.leafApproversMap[path]
d := filepath.Dir(path)
if d == "." {
d = ""
}

return f.leafApproversMap[d]
}

func (f FakeRepo) FindApproverOwnersForFile(path string) string {
Expand Down Expand Up @@ -136,45 +146,45 @@ func TestCreateFakeRepo(t *testing.T) {

tests := []struct {
testName string
ownersFile string
changedFile string
expectedLeafApprovers sets.String
expectedApprovers sets.String
}{
{
testName: "Root Owners",
ownersFile: "",
changedFile: "",
expectedApprovers: rootApprovers,
expectedLeafApprovers: rootApprovers,
},
{
testName: "A Owners",
ownersFile: "a",
changedFile: "a/file.txt",
expectedLeafApprovers: aApprovers,
expectedApprovers: aApprovers.Union(rootApprovers),
},
{
testName: "B Owners",
ownersFile: "b",
changedFile: "b/file.txt",
expectedLeafApprovers: bApprovers,
expectedApprovers: bApprovers.Union(rootApprovers),
},
{
testName: "C Owners",
ownersFile: "c",
changedFile: "c/file.txt",
expectedLeafApprovers: cApprovers,
expectedApprovers: cApprovers.Union(rootApprovers),
},
{
testName: "Combo Owners",
ownersFile: "a/combo",
changedFile: "a/combo/file.txt",
expectedLeafApprovers: edcApprovers,
expectedApprovers: edcApprovers.Union(aApprovers).Union(rootApprovers),
},
}

for _, test := range tests {
calculatedLeafApprovers := fakeRepo.LeafApprovers(test.ownersFile)
calculatedApprovers := fakeRepo.Approvers(test.ownersFile)
calculatedLeafApprovers := fakeRepo.LeafApprovers(test.changedFile)
calculatedApprovers := fakeRepo.Approvers(test.changedFile)

test.expectedLeafApprovers = setToLower(test.expectedLeafApprovers)
if !calculatedLeafApprovers.Equal(test.expectedLeafApprovers) {
Expand Down Expand Up @@ -213,25 +223,26 @@ func TestGetLeafApprovers(t *testing.T) {
{
testName: "Single Root File PR",
filenames: []string{"kubernetes.go"},
expectedMap: map[string]sets.String{"": setToLower(rootApprovers)},
expectedMap: map[string]sets.String{"kubernetes.go": setToLower(rootApprovers)},
},
{
testName: "Internal Node File PR",
filenames: []string{"a/test.go"},
expectedMap: map[string]sets.String{"a": setToLower(aApprovers)},
expectedMap: map[string]sets.String{"a/test.go": setToLower(aApprovers)},
},
{
testName: "Two Leaf File PR",
filenames: []string{"a/d/test.go", "b/test.go"},
expectedMap: map[string]sets.String{
"a/d": setToLower(dApprovers),
"b": setToLower(bApprovers)},
"a/d/test.go": setToLower(dApprovers),
"b/test.go": setToLower(bApprovers)},
},
{
testName: "Leaf and Parent 2 File PR",
filenames: []string{"a/test.go", "a/d/test.go"},
expectedMap: map[string]sets.String{
"a": setToLower(aApprovers),
"a/test.go": setToLower(aApprovers),
"a/d/test.go": setToLower(dApprovers),
},
},
}
Expand Down Expand Up @@ -506,37 +517,37 @@ func TestFindMostCoveringApprover(t *testing.T) {
{
testName: "Single Root File PR",
filenames: []string{"kubernetes.go"},
unapproved: sets.NewString(""),
unapproved: sets.NewString("kubernetes.go"),
expectedMostCovering: setToLower(rootApprovers),
},
{
testName: "Internal Node File PR",
filenames: []string{"a/test.go"},
unapproved: sets.NewString("a"),
unapproved: sets.NewString("a/test.go"),
expectedMostCovering: setToLower(aApprovers),
},
{
testName: "Combo and Intersecting Leaf PR",
filenames: []string{"a/combo/test.go", "a/d/test.go"},
unapproved: sets.NewString("a/combo", "a/d"),
unapproved: sets.NewString("a/combo/test.go", "a/d/test.go"),
expectedMostCovering: setToLower(edcApprovers.Intersection(dApprovers)),
},
{
testName: "Three Leaf PR Only B Approved",
filenames: []string{"a/combo/test.go", "c/test.go", "b/test.go"},
unapproved: sets.NewString("a/combo", "c/"),
unapproved: sets.NewString("a/combo/test.go", "c/test.go"),
expectedMostCovering: setToLower(edcApprovers.Intersection(cApprovers)),
},
{
testName: "Three Leaf PR Only B Left Unapproved",
filenames: []string{"a/combo/test.go", "a/d/test.go", "b/test.go"},
unapproved: sets.NewString("b"),
unapproved: sets.NewString("b/test.go"),
expectedMostCovering: setToLower(bApprovers),
},
{
testName: "Leaf and Parent 2 File PR",
filenames: []string{"a/test.go", "a/d/test.go"},
unapproved: sets.NewString("a", "a/d"),
unapproved: sets.NewString("a/test.go", "a/d/test.go"),
expectedMostCovering: setToLower(aApprovers.Union(dApprovers)),
},
}
Expand Down Expand Up @@ -583,21 +594,21 @@ func TestGetReverseMap(t *testing.T) {
testName: "Single Root File PR",
filenames: []string{"kubernetes.go"},
expectedRevMap: map[string]sets.String{
"alice": sets.NewString(""),
"bob": sets.NewString(""),
"alice": sets.NewString("kubernetes.go"),
"bob": sets.NewString("kubernetes.go"),
},
},
{
testName: "Two Leaf PRs",
filenames: []string{"a/combo/test.go", "a/d/test.go"},
expectedRevMap: map[string]sets.String{
"david": sets.NewString("a/d", "a/combo"),
"dan": sets.NewString("a/d", "a/combo"),
"debbie": sets.NewString("a/d", "a/combo"),
"eve": sets.NewString("a/combo"),
"erin": sets.NewString("a/combo"),
"chris": sets.NewString("a/combo"),
"carol": sets.NewString("a/combo"),
"david": sets.NewString("a/combo/test.go", "a/d/test.go"),
"dan": sets.NewString("a/combo/test.go", "a/d/test.go"),
"debbie": sets.NewString("a/combo/test.go", "a/d/test.go"),
"eve": sets.NewString("a/combo/test.go"),
"erin": sets.NewString("a/combo/test.go"),
"chris": sets.NewString("a/combo/test.go"),
"carol": sets.NewString("a/combo/test.go"),
},
},
}
Expand Down