Migrate label-unapproved-picks from mungegithub to prow #9128

nikhita · 2018-08-22T19:04:51Z

This PR migrates the label-unapproved-picks munger from mungegithub to prow. Ref: #3331.

The original munger did not contain any tests. This PR also adds tests for it.

This PR also makes the plugin configurable:

The branch name against which PRs should be treated as cherrypicks can now be specified as a regexp.
The comment that the plugin comments with while adding the do-not-merge/cherry-pick-not-approved label can be specified.

This PR also updates the label name from cherrypick-unapproved to cherry-pick-unapproved.

The content below this line was originally what the PR was based on. This PR contains the following change as well i.e. it updates the comment body to list the patch release managers.

Ref: kubernetes/community#994 (comment). Note: this does not assign the patch release manager, just suggests them so that it's not too noisy for the release manager. Sometimes it's also useful to assign the release manager only after the cherrypick PR has received lgtm + approval.

Part of #1795

If a cherrypick PR does not have the cherrypick-approved label, the comment (by the bot) just lists that this label does not exist on the PR and adds a do-not-merge/cherry-pick-not-approved label. It does not tell us what needs to be done for cherrypick approval. See kubernetes/kubernetes#67616 (comment) for an example.

This PR updates the comment to point to the list of patch release managers so the author of the PR can know whom to assign to get the cherrypick approved.

/sig release
/sig contributor-experience

nikhita · 2018-08-22T19:05:35Z

/cc @cblecker @guineveresaenger
contribex 🦇 signal

/cc @tpepper @spiffxp

spiffxp · 2018-08-22T19:17:55Z

I get the utility, but I also really don't want any more mungegithub maintenance. Can we instead port this to a prow plugin?

nikhita · 2018-08-22T19:24:06Z

Can we instead port this to a prow plugin?

@spiffxp would this mean porting all the cherrypick-related components here: https://github.com/kubernetes/test-infra/tree/master/mungegithub/mungers?

spiffxp · 2018-08-22T19:25:26Z

overhauling cherrypicking is a much larger task, not trying to blow your scope (and I want to discuss this with patch managers)

I think I'm just asking to port this munger

EDIT: yes, I am, label-unapproved-picks is the only cherrypick related munger that's actively deployed by misc-mungers. The larger discussion we need to have is whether http://cherrypick.k8s.io/ is used by any of the patch managers

nikhita · 2018-08-22T19:34:03Z

@spiffxp 👍

I'll take a dab at porting it to prow tomorrow (it's 1am here 🙃)

spiffxp · 2018-08-22T21:12:37Z

Really appreciate it. If it turns out to be too much of a pain, and this is really necessary to move us forward, we can look at merging. But I think this is the (or very nearly the) last piece of mungegithub to be ported over. And the bonfire will be glorious.

nikhita · 2018-08-23T15:22:45Z

@spiffxp Done!

This original munger did not contain any tests. This PR adds tests too.

PS: I got confused by your edit 🙈 and ported only the label-unapproved-picks munger. Let me know if you were looking to port all of them!

nikhita · 2018-08-23T15:31:32Z

mungegithub/mungers/cherrypick-label-unapproved.go

-	obj.WriteComment(labelUnapprovedBody)
-}
-
-func (LabelUnapprovedPicks) isStaleIssueComment(obj *github.MungeObject, comment *githubapi.IssueComment) bool {


I'm not sure what this does? I didn't add anything related to this to the plugin for now.

This was the old comment cleanup mechanism

nikhita · 2018-08-23T15:32:02Z

prow/plugins/cherrypick/cherrypick-unapproved.go

+// Package cherrypick adds the `do-not-merge/cherry-pick-not-approved`
+// label to PRs against a release branch which do not have the
+// `cherrypick-approved` label.
+package cherrypick


Should the name be cherrypick-unapproved or something like that? (ditto for pluginName)

We an external cherrypicker plugin that implements a /cherrypick command so yeah, this package/plugin should be named something else to avoid confusion.

Not sure what off the top of my head though. Will review and come back

cherrypick-unapproved sounds reasonable as a plugin name. Package names can't have hyphens though so the package would be cherrypickunapproved.

nikhita · 2018-08-23T15:32:46Z

prow/plugins/cherrypick/cherrypick-unapproved.go

+
+func handlePR(gc githubClient, log *logrus.Entry, pr *github.PullRequestEvent) error {
+	// Only consider the events that indicate opening of the PR
+	if pr.Action != github.PullRequestActionOpened && pr.Action != github.PullRequestActionReopened {


PTAL if this right.

This also needs to handle events where pr.Action is the label or unlabel action and pr.Label is the cherrypick-approved label.

cjwagner

Just a couple nits.
/lgtm
/hold

cjwagner · 2018-08-24T20:24:00Z

prow/plugins/cherrypickunapproved/cherrypick-unapproved.go

+}
+
+// shouldPrune finds comments left by this plugin.
+func shouldPrune(log *logrus.Entry, commentBody string) func(github.IssueComment) bool {


log is not used.
You could just get rid of this entire function now and pass this value to PruneComments instead:

func(comment github.IssueComment) bool { return strings.Contains(comment.Body, comment) }

cjwagner · 2018-08-24T20:25:48Z

prow/plugins/cherrypickunapproved/cherrypick-unapproved.go

+	labels, err := gc.GetIssueLabels(org, repo, pr.Number)
+	if err != nil {
+		log.WithError(err).Errorf("failed to list labels")
+		return err


The returned error will be logged so the log message here is redundant.

cjwagner

Thanks again for migrating this!
/lgtm

k8s-ci-robot · 2018-08-24T23:49:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, nikhita

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~label_sync/OWNERS~~ [cjwagner]
~~mungegithub/OWNERS~~ [cjwagner]
~~prow/OWNERS~~ [cjwagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cjwagner · 2018-08-24T23:49:57Z

/hold cancel

k8s-ci-robot · 2018-08-24T23:57:16Z

@nikhita: Updated the label-config configmap using the following files:

key labels.yaml using file label_sync/labels.yaml

In response to this:

This PR migrates the label-unapproved-picks munger from mungegithub to prow. Ref: #3331.

The original munger did not contain any tests. This PR also adds tests for it.

This PR also makes the plugin configurable:

The branch name against which PRs should be treated as cherrypicks can now be specified as a regexp.

The comment that the plugin comments with while adding the do-not-merge/cherry-pick-not-approved label can be specified.

This PR also updates the label name from cherrypick-unapproved to cherry-pick-unapproved.

The content below this line was originally what the PR was based on. This PR contains the following change as well i.e. it updates the comment body to list the patch release managers.

Ref: kubernetes/community#994 (comment). Note: this does not assign the patch release manager, just suggests them so that it's not too noisy for the release manager. Sometimes it's also useful to assign the release manager only after the cherrypick PR has received lgtm + approval.

Part of #1795

If a cherrypick PR does not have the cherrypick-approved label, the comment (by the bot) just lists that this label does not exist on the PR and adds a do-not-merge/cherry-pick-not-approved label. It does not tell us what needs to be done for cherrypick approval. See kubernetes/kubernetes#67616 (comment) for an example.

This PR updates the comment to point to the list of patch release managers so the author of the PR can know whom to assign to get the cherrypick approved.

/sig release
/sig contributor-experience

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.

nikhita · 2018-08-26T09:59:00Z

Cherry-picks are not getting merged right now because the cherry-pick labels were updated from cherrypick-approved to cherry-pick-approved. The label changes were made across the repos immediately (#9128 (comment)).

However, prow was not bumped. The current prow code looks for a cherrypick-approved label and if it doesn't find it, it applies the do-not-merge/cherry-pick-not-approved label. Once prow is bumped, the code will look for the cherry-pick-approved label and remove the do-not-merge label.

Also, even after prow is bumped, all such PRs (https://github.com/kubernetes/kubernetes/labels/cherry-pick-approved) will need a trigger in terms of changing the cherrypick labels or closing/reopening the PR. I'll take care of closing+reopening all the affected ones. 👍

Can some someone bump prow to get this fixed?

stevekuznetsov · 2018-08-26T16:47:52Z

Firstly, thanks @nikhita for the migration!! The work is very appreciated. For the label migration, we can use the label_sync tool to migrate one label to another. Hopefully the doc is sufficient to explain how the config should be changed, but otherwise @fejta is a good contact.

stevekuznetsov · 2018-08-26T16:49:37Z

(But of course a bump to Prow is also necessary, contact the on-call for that.)

nikhita · 2018-08-26T18:46:58Z

For the label migration, we can use the label_sync tool to migrate one label to another.

@stevekuznetsov The change to labels.yaml was done in this PR and the labels have already been migrated. :) Ref: #9128 (comment).

(But of course a bump to Prow is also necessary, contact the on-call for that.)

Have reached out on Slack about this. 👍

k8s-ci-robot requested review from foxish and lavalamp August 22, 2018 19:05

k8s-ci-robot requested review from cblecker, guineveresaenger, spiffxp and tpepper August 22, 2018 19:05

nikhita force-pushed the cherrypick-approve-suggest-patchmanager branch from ec47bb2 to 74bae8c Compare August 22, 2018 19:16

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 22, 2018

nikhita force-pushed the cherrypick-approve-suggest-patchmanager branch from 74bae8c to 4476950 Compare August 23, 2018 15:18

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2018

nikhita changed the title ~~mungegithub: point to list of patch release managers~~ Migrate label-unapproved-picks from mungegithub to prow Aug 23, 2018

nikhita force-pushed the cherrypick-approve-suggest-patchmanager branch from 4476950 to 7ba56e0 Compare August 23, 2018 15:29

nikhita commented Aug 23, 2018

View reviewed changes

k8s-ci-robot assigned cjwagner Aug 24, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018

cjwagner reviewed Aug 24, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2018

nikhita added 3 commits August 25, 2018 02:07

prow: add cherrypick-unapproved plugin

53c77ce

prow: make cherrypick-unapproved configurable

aa20dfc

label_sync: update cherry-pick labels

030e08a

nikhita force-pushed the cherrypick-approve-suggest-patchmanager branch from 79afa70 to 030e08a Compare August 24, 2018 20:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2018

cjwagner approved these changes Aug 24, 2018

View reviewed changes

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018

k8s-ci-robot merged commit 6d9d2ba into kubernetes:master Aug 24, 2018

nikhita mentioned this pull request Aug 26, 2018

Automated cherry pick of #67562: Prevent resourceVersion updates for custom resources on no-op writes kubernetes/kubernetes#67616

Merged

This was referenced Aug 26, 2018

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

Closed

Enable cherrypickunapprove for k/k #9172

Merged

prow/hook: add cherrypickunapproved plugin #9173

Closed

This was referenced Aug 29, 2018

Merge changes from test-infra upstream jetstack/test-infra#246

Merged

Move all prow job configs into config/ jetstack/test-infra#247

Merged

nikhita mentioned this pull request Nov 22, 2018

Other unanswered questions in cherry-picks.md kubernetes/community#1894

Closed

nikhita deleted the cherrypick-approve-suggest-patchmanager branch November 22, 2018 08:22

nikhita mentioned this pull request Dec 3, 2018

How to cherry pick a fix to multiple release branches? kubernetes/community#994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate label-unapproved-picks from mungegithub to prow #9128

Migrate label-unapproved-picks from mungegithub to prow #9128

nikhita commented Aug 22, 2018 •

edited

Loading

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018 •

edited

Loading

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018

nikhita commented Aug 23, 2018 •

edited

Loading

nikhita Aug 23, 2018

stevekuznetsov Aug 24, 2018

nikhita Aug 23, 2018 •

edited

Loading

spiffxp Aug 23, 2018

cjwagner Aug 23, 2018

nikhita Aug 23, 2018

cjwagner Aug 23, 2018

cjwagner left a comment

cjwagner Aug 24, 2018

nikhita Aug 24, 2018

cjwagner Aug 24, 2018

nikhita Aug 24, 2018

cjwagner left a comment

k8s-ci-robot commented Aug 24, 2018

cjwagner commented Aug 24, 2018

k8s-ci-robot commented Aug 24, 2018

nikhita commented Aug 26, 2018 •

edited

Loading

stevekuznetsov commented Aug 26, 2018

stevekuznetsov commented Aug 26, 2018

nikhita commented Aug 26, 2018

Migrate label-unapproved-picks from mungegithub to prow #9128

Migrate label-unapproved-picks from mungegithub to prow #9128

Conversation

nikhita commented Aug 22, 2018 • edited Loading

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018 • edited Loading

nikhita commented Aug 22, 2018

spiffxp commented Aug 22, 2018

nikhita commented Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita Aug 23, 2018 • 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

cjwagner 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

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 24, 2018

cjwagner commented Aug 24, 2018

k8s-ci-robot commented Aug 24, 2018

nikhita commented Aug 26, 2018 • edited Loading

stevekuznetsov commented Aug 26, 2018

stevekuznetsov commented Aug 26, 2018

nikhita commented Aug 26, 2018

nikhita commented Aug 22, 2018 •

edited

Loading

spiffxp commented Aug 22, 2018 •

edited

Loading

nikhita commented Aug 23, 2018 •

edited

Loading

nikhita Aug 23, 2018 •

edited

Loading

nikhita commented Aug 26, 2018 •

edited

Loading