Port docs-no-retest from munge to prow #4895

paradigm · 2017-10-05T20:00:05Z

From what I understand, there is an effort to move from munge to prow for github integration. This ports munge's docs-no-retest functionality to prow. It also adds test coverage, as munge's docs-no-retest was not tested.

/cc @Kargakis @stevekuznetsov
/assign @Kargakis

k8s-ci-robot · 2017-10-05T20:00:13Z

Hi @paradigm. 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.

BenTheElder · 2017-10-05T20:02:35Z

Awesome!
/assign @cjwagner
/ok-to-test
/area prow
/area mungegithub

cjwagner · 2017-10-05T20:56:48Z

prow/plugins/docs-no-retest/docs-no-retest.go

+)
+
+var (
+	docFilesRegex    = regexp.MustCompile("^.*\\.(md|png|svg|dia)$")


Using back ticks instead of quotes for regexps will avoid the need to double escape. `^.*\.(md|png|svg|dia)$`

cjwagner · 2017-10-05T21:01:14Z

prow/plugins/docs-no-retest/docs-no-retest.go

+	GetPullRequestChanges(org, repo string, number int) ([]github.PullRequestChange, error)
+}
+
+func handlePR(gc githubClient, pe github.PullRequestEvent) error {


This function should start by checking that the event's Action is opened, reopened, or synchronized, and return early if not.

cjwagner · 2017-10-05T21:11:54Z

prow/plugins/docs-no-retest/docs-no-retest.go

+
+	if docsOnly && !hasTargetLabel {
+		if err := gc.AddLabel(owner, repo, num, labelSkipRetest); err != nil {
+			return fmt.Errorf("error adding label to %s/%s PR #%d: %v", owner, repo, num, err)


The convention Prow uses for referencing github issues is fmt.Sprintf("%s/%s#%d", owner, repo, num)

cjwagner · 2017-10-05T21:14:29Z

prow/plugins/docs-no-retest/docs-no-retest_test.go

+					Filename: "/path/to/file/foo.go",
+				},
+			},
+			getIssueLabelsErr: testError,


s/getIssueLabelsErr/getPullRequestChangesErr/

cjwagner · 2017-10-05T21:17:33Z

prow/plugins/docs-no-retest/docs-no-retest_test.go

+
+type ghc struct {
+	*testing.T
+	labels    map[github.Label]bool


Just store the label name instead of the entire Label struct since the Color and ID fields are ignored.

paradigm · 2017-10-06T17:35:32Z

All those changes made sense to me, applied here.

stevekuznetsov · 2017-10-06T17:38:52Z

prow/plugins/docs-no-retest/docs-no-retest.go

+		num   = pe.PullRequest.Number
+	)
+
+	switch pe.Action {


Generally more idiomatic golang to do this with an if statement like here

stevekuznetsov · 2017-10-06T17:41:12Z

prow/plugins/docs-no-retest/docs-no-retest_test.go

+	c.T.Logf("RemoveLabel: %s", targetLabel)
+	for label := range c.labels {
+		if label == targetLabel {
+			delete(c.labels, label)


You're doing a deletion here so you don't really need the bool, can just use map[string]interface{} (the k8s set.String utility)

cjwagner · 2017-10-06T20:09:40Z

The configs need to be updated as well.
Please remove docs-need-no-retest from the kubernetes misc-mungers config and also add the docs-no-retest plugin to the kubernetes/kubernetes repo in plugins.yaml.

This will also require a hook bump. Please run ./bump.sh hook from the prow directory and commit the changes to a new commit.

stevekuznetsov · 2017-10-06T20:11:07Z

[k8sCI] migrate the retest-not-required-docs-only munger to Prow

cjwagner · 2017-10-06T20:31:16Z

Looks like you need to rebase before running ./bump.sh hook.

cjwagner · 2017-10-06T20:48:55Z

https://github.com/kubernetes/test-infra/blob/master/prow/hook/plugins.go needs to be updated.

0xmichalis · 2017-10-09T13:19:28Z

prow/plugins/docs-no-retest/docs-no-retest_test.go

+			err := handlePR(client, event)
+
+			if err != nil && c.err == nil {
+				t.Errorf("test case \"%s\": unexpected handlePR error: %v", c.name, err)


nit: you can use %q instead of "%s"

0xmichalis · 2017-10-09T13:19:41Z

/lgtm

0xmichalis · 2017-10-09T13:24:17Z

prow/plugins.yaml

@@ -51,6 +51,7 @@ plugins:
  kubernetes/kubernetes:
  - trigger
  - release-note
+  - docs-no-retest


@cjwagner I am sure you already know that this will require you to redeploy instantly.

Yep, I'll merge and redeploy Prow and the kubernetes misc-mungers deployment now.
/approve

k8s-github-robot · 2017-10-09T18:09:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, kargakis, paradigm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~mungegithub/OWNERS~~ [cjwagner]
~~prow/OWNERS~~ [cjwagner,kargakis]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-ci-robot · 2017-10-09T18:16:45Z

@paradigm: I updated Prow plugins config for you!

In response to this:

From what I understand, there is an effort to move from munge to prow for github integration. This ports munge's docs-no-retest functionality to prow. It also adds test coverage, as munge's docs-no-retest was not tested.

/cc @Kargakis @stevekuznetsov
/assign @Kargakis

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.

paradigm requested a review from cjwagner as a code owner October 5, 2017 20:00

k8s-ci-robot assigned 0xmichalis Oct 5, 2017

k8s-ci-robot requested review from 0xmichalis and stevekuznetsov October 5, 2017 20:00

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 5, 2017

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 5, 2017

k8s-ci-robot assigned cjwagner Oct 5, 2017

k8s-ci-robot added area/prow Issues or PRs related to prow area/mungegithub and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 5, 2017

cjwagner reviewed Oct 5, 2017

View reviewed changes

paradigm force-pushed the docs-no-retest-munge-to-prow branch from 4af474f to ba38c7d Compare October 6, 2017 17:28

stevekuznetsov reviewed Oct 6, 2017

View reviewed changes

paradigm force-pushed the docs-no-retest-munge-to-prow branch from ba38c7d to ee12b73 Compare October 6, 2017 18:22

paradigm force-pushed the docs-no-retest-munge-to-prow branch from ee12b73 to bfbe1f9 Compare October 6, 2017 20:27

paradigm force-pushed the docs-no-retest-munge-to-prow branch from bfbe1f9 to dd2fb44 Compare October 6, 2017 20:35

paradigm added 2 commits October 6, 2017 16:58

Port docs-no-retest from munge to prow

755b70d

Bump hook to 0.172.

87a33a9

paradigm force-pushed the docs-no-retest-munge-to-prow branch from dd2fb44 to 87a33a9 Compare October 6, 2017 20:59

0xmichalis reviewed Oct 9, 2017

View reviewed changes

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

0xmichalis reviewed Oct 9, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2017

cjwagner merged commit 98eec4a into kubernetes:master Oct 9, 2017

paradigm deleted the docs-no-retest-munge-to-prow branch October 9, 2017 19:09

spiffxp mentioned this pull request Dec 11, 2017

Migrate all mungers out of mungegithub to prow plugins / periodic jobs #3331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port docs-no-retest from munge to prow #4895

Port docs-no-retest from munge to prow #4895

paradigm commented Oct 5, 2017

k8s-ci-robot commented Oct 5, 2017

BenTheElder commented Oct 5, 2017

cjwagner Oct 5, 2017

cjwagner Oct 5, 2017

cjwagner Oct 5, 2017

cjwagner Oct 5, 2017

cjwagner Oct 5, 2017

paradigm commented Oct 6, 2017

stevekuznetsov Oct 6, 2017

stevekuznetsov Oct 6, 2017

cjwagner commented Oct 6, 2017

stevekuznetsov commented Oct 6, 2017

cjwagner commented Oct 6, 2017

cjwagner commented Oct 6, 2017

0xmichalis Oct 9, 2017

0xmichalis commented Oct 9, 2017

0xmichalis Oct 9, 2017

cjwagner Oct 9, 2017

k8s-github-robot commented Oct 9, 2017

k8s-ci-robot commented Oct 9, 2017

Port docs-no-retest from munge to prow #4895

Port docs-no-retest from munge to prow #4895

Conversation

paradigm commented Oct 5, 2017

k8s-ci-robot commented Oct 5, 2017

BenTheElder commented Oct 5, 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

paradigm commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner commented Oct 6, 2017

stevekuznetsov commented Oct 6, 2017

cjwagner commented Oct 6, 2017

cjwagner commented Oct 6, 2017

Choose a reason for hiding this comment

0xmichalis commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Oct 9, 2017

k8s-ci-robot commented Oct 9, 2017