Refactor Approval Implementation #1846

grodrigues3 · 2017-02-09T01:26:20Z

No description provided.

apelisse

I'm done for now, I think this can still be vastly simplified

apelisse · 2017-02-09T17:15:58Z

mungegithub/mungers/approvers/owners.go

+	for _, fn := range o.filenames {
+		owners.Insert(o.repo.FindOwnersForPath(fn))
+	}
+	files := owners.List()


sets.String.List() returns a sorted list, no need to re-sort.

apelisse · 2017-02-09T17:21:49Z

mungegithub/mungers/approvers/owners.go

+
+func (o Owners) IsApproved(currentlyApproved sets.String) bool {
+	fnMap := o.GetApprovers() //ownersfiles -> Relevant Approvers
+	for _, file := range o.ListOwnersFiles() {


why don't you loop over fnmap?

apelisse · 2017-02-09T17:22:23Z

mungegithub/mungers/approvers/owners.go

+
+// removeSubdirs takes a list of directories as an input and returns a set of directories with all
+// subdirectories removed.  E.g. [/a,/a/b/c,/d/e,/d/e/f] -> [/a, /d/e]
+func removeSubdirs(dirList []string) sets.String {


This is not used yet?

apelisse · 2017-02-09T17:25:03Z

mungegithub/mungers/approvers/owners.go

+	}
+	files := owners.List()
+	sort.Strings(files)
+	return files


Maybe we should return the set rather than the list here.

apelisse · 2017-02-09T17:25:28Z

mungegithub/mungers/approvers/owners.go

+	copyOfFiles := sets.NewString()
+	for _, fn := range o.filenames {
+		copyOfFiles.Insert(fn)
+	}


This could be replaced by ListOwnersFiles (especially if we make it return a set)

apelisse · 2017-02-09T17:38:22Z

mungegithub/mungers/approvers/owners.go

+	for approver := range approverOwnersfiles {
+		sliceOfKeys = append(sliceOfKeys, approver)
+	}
+	sort.Strings(sliceOfKeys)


Maybe make a function for that GetAllPotentialApprovers()

apelisse · 2017-02-09T17:42:01Z

mungegithub/mungers/approvers/owners.go

+		people = append(people, sliceOfKeys[order[i]])
+	}
+
+	files = []sets.String{}


The following code then can be removed

apelisse · 2017-02-09T17:42:45Z

mungegithub/mungers/approvers/owners.go

+		sliceOfKeys = append(sliceOfKeys, approver)
+	}
+	sort.Strings(sliceOfKeys)
+	order := rand.Perm(len(approverOwnersfiles))


These 4 lines are the only thing left

apelisse · 2017-02-09T17:50:04Z

mungegithub/mungers/approvers/owners.go

+		toDelete := sets.NewString()
+		// remove all files in the directories that our approver approved AND
+		// in the subdirectories that s/he approved.  HasPrefix finds subdirs
+		for fn := range copyOfFiles {


I don't exactly get that.

At this point you have the suggested person, don't you just want to remove from copyOfFiles (which is a terrible name) the list of files that person can approve? copyOfFiles = copyOfFiles.Difference(filesCanApprove[bestPerson]) and remove a bunch of temporary?

suppose for some reason we choose Tim or Brian as the best person. They are only in /OWNERS but not /pkg/api/OWNERS. Checking the prefix makes sure that we not only include the fact that they cover the root, but also all subdirs. Does that make sense?

apelisse · 2017-02-09T17:52:27Z

mungegithub/mungers/approvers/owners.go

+	var bestPerson string
+	var bestPersonFiles sets.String
+	for copyOfFiles.Len() > 0 {
+		maxCovered := 0


The following 8-10 lines is basically: FindPersonWithBiggestCoverage(toBeApproved, randomizedApprovers, reverseMap) (person string)

apelisse · 2017-02-09T18:02:39Z

Many functions are really only related to finding the list of suggested approvers, I'm wondering if it's a sign that we should cut a new type there.

grodrigues3 · 2017-02-09T21:58:11Z

mungegithub/mungers/approvers/owners.go

+	revMap := o.GetReverseMap()
+
+	setToCover := sets.NewString()
+	for _, fn := range o.filenames {


better variable name?

grodrigues3 · 2017-02-09T21:58:42Z

mungegithub/mungers/approvers/owners.go

+
+	approverGroup := sets.NewString()
+	for setToCover.Len() > 0 {
+		bestPerson := findPersonWithBiggestCoverage(setToCover, randomizedApprovers, revMap)


turned it into a function.

grodrigues3 · 2017-02-09T22:35:09Z

mungegithub/mungers/approvers/owners.go

+// IsApproved returns a bool indicating if the entire PR is approved
+func (o Owners) IsApproved(currentlyApproved sets.String) bool {
+	//o.GetApprovers returns map ownersfiles -> Relevant Approvers
+	for _, approvers := range o.GetApprovers() {


This isn't right. o.GetApprovers returns the leafApprovers only. We also need to check further up the tree.

grodrigues3 · 2017-02-10T18:37:04Z

@apelisse ping. what do you think about the tests?

grodrigues3 · 2017-02-15T00:55:33Z

mungegithub/mungers/approvers/owners.go

+	unapprovedFiles := o.UnapprovedFiles(alreadyApproved)
+	for _, path := range o.GetOwnersSet().List() {
+		fullOwnersPath := filepath.Join(path, ownersFileName)
+		approverSet := o.repo.Approvers(path).Intersection(alreadyApproved)


Should I create a method attached to owners that returns a map from ownersFiles -> people that have approved them?

grodrigues3 · 2017-02-15T00:56:16Z

mungegithub/mungers/approvers/owners.go

+	}
+	context.WriteString("\n")
+	suggestedApprovers := o.GetSuggestedApprovers()
+	toBeAssigned := sets.NewString(suggestedApprovers...).Difference(alreadyApproved)


Function for getting people that should be assigned or is it okay to have the logic here?

apelisse · 2017-02-16T19:22:54Z

mungegithub/mungers/approvers/owners.go

+
+type UnapprovedFile struct {
+	filepath  string
+	approvers sets.String


This is not needed, that's the beauty of it.

apelisse · 2017-02-16T19:25:33Z

mungegithub/mungers/approvers/owners.go

+// 	- how an approver can indicate their approval
+// 	- how an approver can cancel their approval
+func getMessage(ap Approvers, org, project string) string {
+	context := bytes.NewBufferString("The following people have approved this PR: ")


Nit: can we turn that into a big string?

apelisse · 2017-02-21T17:29:01Z

mungegithub/mungers/approvers/approvers_test.go

+			testName:           "Single Root File PR No One Approved",
+			filenames:          []string{"kubernetes.go"},
+			currentlyApproved:  sets.NewString(),
+			expectedUnapproved: sets.NewString(""),


Empty string means root, that's kind of weird

yeah, but that's what github sends us back. E.g the makefile in the root dir is listed as Makefile not /Makefile

apelisse · 2017-02-21T17:30:57Z

mungegithub/mungers/approvers/approvers_test.go

+			testName:           "Combo and Other; Combo Approved",
+			filenames:          []string{"a/combo/test.go", "a/d/test.go"},
+			currentlyApproved:  eApprovers,
+			expectedUnapproved: sets.NewString("a/d"),


Reading this test is far from obvious. eApprovers has approved, and somehow a/combo is approved ... Following the top structure helps, but it's still puzzling

apelisse · 2017-02-21T17:31:44Z

mungegithub/mungers/approvers/approvers_test.go

+			testName:           "B Only UnApproved",
+			filenames:          []string{"b/test_1.go", "b/test.go"},
+			currentlyApproved:  bApprovers,
+			expectedUnapproved: sets.NewString(),


Do we have a test for:

filenames: []string{"b/test_1.go", "b/test.go"}, currentlyApproved: rootApprovers,

apelisse · 2017-02-21T17:32:54Z

mungegithub/mungers/approvers/approvers_test.go

+		{
+			testName:           "Combo and Other; Both Approved",
+			filenames:          []string{"a/combo/test.go", "a/d/test.go"},
+			currentlyApproved:  edcApprovers.Intersection(dApprovers),


OK I have no idea what this is. Isn't ecdApprovers.Intersection(dApprovers) equal to dApprovers?

yeah it is. I added that so it's clear that the dApprovers are present in both sets (otherwise the intersection would be empty set. better to just be dApprovers?

apelisse · 2017-02-21T17:34:55Z

mungegithub/mungers/approvers/approvers_test.go

+			filenames:         []string{"a/combo/test.go", "a/d/test.go", "c/test"},
+			currentlyApproved: cApprovers,
+			expectedFiles:     []File{ApprovedFile{"a/combo", cApprovers}, UnapprovedFile{"a/d"}, ApprovedFile{"c", cApprovers}},
+		},


You have no test where a group of people approve a dir, and another group approves another dir, and maybe a group approves multiple dirs.

I don't understand this. Can you elaborate more?

Look at your tests, you always have only one set of approvers.
You don't have a test like:

filenames: []string{"a/something", "b/something"} currentlyApproved: aApprovers, bApprovers

apelisse · 2017-02-21T17:35:57Z

mungegithub/mungers/approvers/approvers_test.go

+		filenames         []string
+		currentlyApproved sets.String
+		// testSeed affects who is chosen for CC
+		testSeed int64


is there any test where the seed has an impact? Are you testing that different seeds have different results?

yeah, the GetCCs test

apelisse · 2017-02-21T17:39:10Z

mungegithub/mungers/approvers/owners.go

+func (ap Approvers) UnapprovedFiles() sets.String {
+	unapproved := sets.NewString()
+	for fn := range ap.owners.GetOwnersSet() {
+		approvers := ap.owners.repo.Approvers(fn)


I really don't think you should access members from owners directly

grodrigues3 · 2017-02-21T18:23:36Z

mungegithub/mungers/approval-handler.go

@@ -159,7 +143,7 @@ func getApproveComments(comments []*githubapi.IssueComment) c.FilteredComments {
 	return c.FilterComments(comments, c.Or{approverMatcher, lgtmMatcher})
 }

-func (h *ApprovalHandler) updateNotification(obj *github.MungeObject, ownersMap map[string]sets.String, approverSet sets.String, isFullyApproved bool) error {
+func (h *ApprovalHandler) updateNotification(obj *github.MungeObject, approversHandler approvers.Approvers) error {


how do we fix this?

grodrigues3 · 2017-03-01T17:24:08Z

mungegithub/mungers/approvers/approvers_test.go

+			expectedUnapproved: sets.NewString(),
+		},
+		{
+			testName:           "B Files Approved at Root",


Here's the root test

grodrigues3 · 2017-03-01T18:07:18Z

@k8s-bot bazel test this

apelisse · 2017-03-01T18:48:05Z

Once you've validated on your repo that everything is fine, please feel free to merge.

/lgtm

- Redesign with multiple classes each with specific behaviors - Modular design and comprehensive testing

grodrigues3 added the do-not-merge label Feb 9, 2017

grodrigues3 assigned apelisse Feb 9, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 9, 2017

grodrigues3 force-pushed the refactor-approvers-implementation branch 2 times, most recently from a3cb0a6 to ad8d40f Compare February 9, 2017 01:39

apelisse reviewed Feb 9, 2017

View reviewed changes

grodrigues3 commented Feb 9, 2017

View reviewed changes

grodrigues3 force-pushed the refactor-approvers-implementation branch 2 times, most recently from b422f66 to 4123a36 Compare February 10, 2017 18:40

grodrigues3 commented Feb 15, 2017

View reviewed changes

grodrigues3 force-pushed the refactor-approvers-implementation branch from 64daca8 to b3ebae1 Compare February 16, 2017 01:11

apelisse reviewed Feb 16, 2017

View reviewed changes

apelisse reviewed Feb 21, 2017

View reviewed changes

grodrigues3 force-pushed the refactor-approvers-implementation branch 2 times, most recently from 92f37a9 to e981108 Compare February 21, 2017 18:21

grodrigues3 commented Feb 21, 2017

View reviewed changes

fejta assigned rmmh Feb 24, 2017

grodrigues3 force-pushed the refactor-approvers-implementation branch 2 times, most recently from 4d68cf5 to 0b2a82d Compare February 24, 2017 18:47

apelisse mentioned this pull request Feb 28, 2017

mungebot/approval: cc only applicable approvers #2076

Closed

grodrigues3 force-pushed the refactor-approvers-implementation branch from 0b2a82d to 6bfbe72 Compare March 1, 2017 17:23

grodrigues3 commented Mar 1, 2017

View reviewed changes

grodrigues3 force-pushed the refactor-approvers-implementation branch from 73f771c to b2f5f77 Compare March 1, 2017 18:42

grodrigues3 force-pushed the refactor-approvers-implementation branch from b2f5f77 to 2e02db9 Compare March 1, 2017 18:46

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017

Refactor Approvers Mechanims

8d1d807

- Redesign with multiple classes each with specific behaviors - Modular design and comprehensive testing

grodrigues3 force-pushed the refactor-approvers-implementation branch from d9dcab9 to 8d1d807 Compare March 1, 2017 21:57

grodrigues3 removed the do-not-merge label Mar 1, 2017

grodrigues3 merged commit 1059565 into kubernetes:master Mar 1, 2017

Refactor Approval Implementation #1846

Refactor Approval Implementation #1846

Conversation

grodrigues3 commented Feb 9, 2017

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grodrigues3 commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grodrigues3 commented Mar 1, 2017

apelisse commented Mar 1, 2017

apelisse Feb 21, 2017 •

edited

Loading