prow: size plugin #3847

nlandolfi · 2017-08-01T22:17:47Z

Implement the size munger as a prow plugin.

Closes #3794, which also contains more context.

/area prow

k8s-ci-robot · 2017-08-01T22:17:54Z

Hi @nlandolfi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

nlandolfi · 2017-08-01T22:32:16Z

At this stage, the PR contains the main logic.

It has a few TODOs in places, some where I'd be curious for feedback.

It lacks test cases for the handlePR function, which I will be adding shortly. But was curious to get early feedback since it's starting to get a bit large...

crimsonfaith91 · 2017-08-02T17:39:08Z

/ok-to-test

spxtr · 2017-08-04T23:18:59Z

prow/plugins/size/gen_files_group.go

+// Use load to read a generated files config file, and populate g with the commands.
+// "paths-from-repo" commands are aggregated into repoPaths. It is the caller's
+// responsiblity to fetch these and load them via g.loadPaths.
+func (g *GenFilesGroup) load(r io.Reader) (repoPaths []string, parseErrs []*ParseError, err *IOError) {


Just return one error. Users of the library can do a type assertion if they care what kind of error it is.

so should it be a list of []error? The catch is that if there is a parse error...and we continue parsing despite it, we could run into another parse error?

err: a slice of errors, i.e., []error

spxtr · 2017-08-04T23:19:31Z

prow/plugins/size/gen_files_group.go

+}
+
+// IOError is related to reading or writing bytes.
+type IOError struct {


This is probably unnecessary. We can just assume that if it's not a ParseError then it's something to worry about.

spxtr · 2017-08-04T23:22:49Z

prow/plugins/size/gen_files_group_test.go

+			// Check g.FileNames
+
+			if got, want := len(g.FileNames), len(c.want.FileNames); got != want {
+				t.Logf("g.FileNames: got %v, want %v", g.FileNames, c.want.FileNames)


I don't think we need the extra log line.

spxtr · 2017-08-04T23:23:15Z

prow/plugins/size/gen_files_group_test.go

+			}
+
+			// Check g.FileNames
+


Remove the extra newlines here and the others as well.

spxtr · 2017-08-04T23:23:58Z

prow/plugins/size/pr.go

@@ -0,0 +1,101 @@
+/*


Put the contents of both this file and "plugin.go" into one file called "size.go".

spxtr · 2017-08-04T23:25:07Z

prow/plugins/size/pr.go

+	)
+
+	// Ignore parseErrors, best effort despite invalid lines in config.
+	g, _, err := NewGenFilesGroup(gc, owner, repo, sha)


Use the logger's Warning method to indicate something that's probably wrong but not on Prow's side.

instead of returning the error, or in addition to returning it?

spxtr · 2017-08-04T23:26:05Z

prow/plugins/size/pr.go

+		count += change.Additions + change.Deletions
+	}
+
+	labels, err := gc.GetIssueLabels(owner, repo, num)


Are these contained in the PullRequestEvent?

They do not appear to be contained here. even tho the structure is massive

spxtr · 2017-08-04T23:27:37Z

prow/plugins/size/test-fixtures/k8s-docs-.generated_docs.paths

@@ -0,0 +1,5 @@
+# first 30 lines of kubernetes/docs/.generated_docs


In go it's normal to call this folder testdata instead of test-fixtures. Various go tools are smart about that name.

spxtr · 2017-08-04T23:27:58Z

/lint

k8s-ci-robot

@spxtr: 14 warnings.

In response to this:

/lint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2017-08-04T23:28:03Z

prow/plugins/size/plugin.go

+limitations under the License.
+*/
+
+package size


Golint comments: should have a package comment, unless it's in another file for this package. More info.

k8s-ci-robot · 2017-08-04T23:28:03Z

prow/plugins/size/pr.go

+	}
+
+	if err := gc.AddLabel(owner, repo, num, newLabel); err != nil {
+		return fmt.Errorf("AddLabel error: %v", err)


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

Feel free to ignore this.

k8s-ci-robot · 2017-08-04T23:28:03Z

prow/plugins/size/pr.go

+limitations under the License.
+*/
+
+package size


Golint comments: should have a package comment, unless it's in another file for this package. More info.

k8s-ci-robot · 2017-08-04T23:28:03Z

prow/plugins/size/pr.go

+
+	changes, err := gc.GetPullRequestChanges(owner, repo, num)
+	if err != nil {
+		return fmt.Errorf("GetPullRequestChanges error: %v", err)


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

Feel free to ignore this.

k8s-ci-robot · 2017-08-04T23:28:03Z

prow/plugins/size/pr.go

+	labels, err := gc.GetIssueLabels(owner, repo, num)
+	if err != nil {
+		// TODO(nlandolfi): should probably continue anyway, to at least try to add label?
+		return fmt.Errorf("GetIssueLabels error: %v", err)


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

Feel free to ignore this.

k8s-ci-robot · 2017-08-04T23:28:04Z

prow/plugins/size/size.go

+limitations under the License.
+*/
+
+package size


Golint comments: should have a package comment, unless it's in another file for this package. More info.

k8s-ci-robot · 2017-08-04T23:28:04Z

prow/plugins/size/size.go

+
+package size
+
+// One of a discrete buckets.


Golint comments: comment on exported type Size should be of the form "Size ..." (with optional leading article). More info.

Unexport this.

k8s-ci-robot · 2017-08-04T23:28:04Z

prow/plugins/size/gen_files_group.go

+limitations under the License.
+*/
+
+package size


Golint comments: should have a package comment, unless it's in another file for this package. More info.

k8s-ci-robot · 2017-08-04T23:28:04Z

prow/plugins/size/gen_files_group.go

+	var bs []byte
+	bs, err = gc.GetFile(owner, repo, genConfigFile, sha)
+	if err != nil {
+		err = &IOError{err: fmt.Errorf("GetFile error: %v", err)}


Golint errors: error strings should not be capitalized or end with punctuation or a newline. More info.

plan to kill this error type anyway

k8s-ci-robot · 2017-08-04T23:28:04Z

prow/plugins/size/gen_files_group.go

+	return nil
+}
+
+// Use Match to determine whether a file, given here by its full path


Golint comments: comment on exported method GenFilesGroup.Match should be of the form "Match ...". More info.

nlandolfi · 2017-08-09T00:02:32Z

@Kargakis joe mentioned you might be able to take a look at this while he was away -- if so that would be great, if you don't have time no worries :) !

0xmichalis · 2017-08-09T07:45:10Z

prow/github/client.go

+	}
+
+	decoded, err := base64.StdEncoding.DecodeString(res.Content)
+


Kill this line

0xmichalis · 2017-08-09T07:49:22Z

prow/plugins/size/gen_files_group.go

+			return g, nil
+		default:
+			err = fmt.Errorf("GetFile error: %v", err)
+			return


Likely a personal taste, but I think it's better to always use return arguments.

ya i actually agree will rework this.

0xmichalis · 2017-08-09T07:52:01Z

prow/plugins/size/gen_files_group.go

+	"k8s.io/test-infra/prow/github"
+)
+
+const genConfigFile = ".generated_files"


Where does this file come from? Can you add a comment explaining its utility?

It would be nice to also include an example of this file's content - you already have one in the unit tests.

i think i'll take some of the language from the issue I wrote describing the file spec cause it'd be nice to have that near the code i think

0xmichalis · 2017-08-09T07:55:40Z

prow/plugins/size/gen_files_group_test.go

+			src: `# Files that should be ignored by tools which do not want to consider generated
+# code.
+#
+# https://github.com/kubernetes/contrib/blob/master/mungegithub/mungers/size.go


This link is dead now. s/contrib/test-infra/

k should we file an issue about it in kubernetes/kubernetes here

i can even just open a pr for kubernetes/kubernetes if it needs to be fixed

Fixing it in kubernetes is fine but a separate issue. This is just a unit test.

0xmichalis · 2017-08-09T08:04:32Z

prow/plugins/size/size.go

+
+	changes, err := gc.GetPullRequestChanges(owner, repo, num)
+	if err != nil {
+		return fmt.Errorf("GetPullRequestChanges error: %v", err)


cannot get PR changes for size plugin: %v

0xmichalis · 2017-08-09T08:07:58Z

prow/plugins/size/gen_files_group_test.go

+		{path: "generated.txt", match: true},
+		{path: "/any/old/path/generated.txt", match: true},
+		{path: "/everywhere/generated.txt", match: true},
+		{path: "/and/nowhere/generated.txt", match: true},


Can you add a case where a filename does not match?

0xmichalis · 2017-08-09T08:10:52Z

prow/plugins/size/size.go

+
+	labels, err := gc.GetIssueLabels(owner, repo, num)
+	if err != nil {
+		le.Warnf("while retrieving labels, error: %v", err)


It seems that if you get an error here, you don't get to run any of the following code so you might as well return here.

i add the label later around line 114 -- maybe should add the label first then remove?

AddLabell will never be reached because if you get an error here, labels is nil and hasLabel stays false.

Oh, not sure why I thought it like that :) - actually AddLabel is run, yeah this is fine.

0xmichalis · 2017-08-09T08:14:19Z

prow/plugins/size/size.go

+	}
+
+	if err := gc.AddLabel(owner, repo, num, newLabel); err != nil {
+		return fmt.Errorf("AddLabel error: %v", err)


cannot add label in %s/%s PR #%d: %v ?

0xmichalis · 2017-08-10T09:38:17Z

You also need to bump hook and squash the commits (big change but I am not sure how to bucket it more efficiently).

nlandolfi · 2017-08-11T00:18:49Z

done!

0xmichalis · 2017-08-11T08:54:21Z

prow/plugins/size/gen_files_group.go

+			repoPaths = append(repoPaths, fs[1])
+		default:
+			err = &ParseError{line: l}
+			return


Can you add the return arguments here, too?

0xmichalis · 2017-08-11T08:54:46Z

prow/plugins/size/gen_files_group.go

+	"k8s.io/test-infra/prow/github"
+)
+
+// The ".generated_files" config lives in the repo's root.


0xmichalis · 2017-08-11T08:55:20Z

prow/plugins/size/gen_files_group.go

+//
+// The statement's `key` specifies the type of the corresponding value:
+//	- "path": exact path to a single file
+//  - "file-name": exact leaf file name, regardless of path


nit: extra whitespace

yeah some were tabs some were spaces, went to all spaces for this list.

0xmichalis · 2017-08-11T08:56:20Z

prow/plugins/size/gen_files_group.go

+		case *github.FileNotFound:
+			return g, nil
+		default:
+			return nil, fmt.Errorf("GetFile error: %v", err)


could not get .generated_files: %v

0xmichalis · 2017-08-11T08:59:38Z

Had a couple of minor comments but this is
/lgtm

nlandolfi · 2017-08-17T02:21:17Z

let me know if there is anything else on my end!

0xmichalis · 2017-08-17T09:09:25Z

@nlandolfi can you open an issue to replace the munger with the new plugin in the existing configs in test-infra? As far as the old code is concerned, I wouldn't kill it yet but I would open a separate issue for it, too.

spxtr · 2017-08-17T23:13:58Z

#4105 (comment)

BenTheElder · 2017-08-17T23:17:29Z

@Kargakis spxtr enabled the plugin for test-infra in #4100 😄

spxtr · 2017-08-18T00:54:08Z

Still need to disable the munger and enable the plugin across all of kubernetes.

nlandolfi requested a review from spxtr as a code owner August 1, 2017 22:17

k8s-ci-robot added area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 1, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 1, 2017

nlandolfi mentioned this pull request Aug 1, 2017

RFC: Size Munger -> Prow "Size" Plugin #3794

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2017

spxtr reviewed Aug 4, 2017

View reviewed changes

k8s-ci-robot reviewed Aug 4, 2017

View reviewed changes

0xmichalis reviewed Aug 9, 2017

View reviewed changes

0xmichalis reviewed Aug 11, 2017

View reviewed changes

0xmichalis changed the title ~~[WIP] prow: size plugin~~ prow: size plugin Aug 11, 2017

k8s-ci-robot assigned 0xmichalis Aug 11, 2017

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

introduce prow/plugins/size

5d75d07

0xmichalis merged commit e9040c9 into kubernetes:master Aug 17, 2017

		@@ -0,0 +1,5 @@
		# first 30 lines of kubernetes/docs/.generated_docs

		}

		decoded, err := base64.StdEncoding.DecodeString(res.Content)

prow: size plugin #3847

prow: size plugin #3847

Conversation

nlandolfi commented Aug 1, 2017

k8s-ci-robot commented Aug 1, 2017

nlandolfi commented Aug 1, 2017

crimsonfaith91 commented Aug 2, 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

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

spxtr commented Aug 4, 2017

k8s-ci-robot 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

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

nlandolfi commented Aug 9, 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

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

0xmichalis commented Aug 10, 2017

nlandolfi commented Aug 11, 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