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.

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

Enable cherrypick plugin for k-sigs/{cluster-api,controller-tools,controller-runtime} #16329

Merged
merged 4 commits into from
Sep 2, 2021

Conversation

droslean
Copy link
Member

@droslean droslean commented Feb 17, 2020

Enable cherrypick plugin for k-sigs/{cluster-api,controller-tools,controller-runtime}

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Feb 17, 2020
@droslean
Copy link
Member Author

/cc @alejandrox1 FYI

@k8s-ci-robot
Copy link
Contributor

@droslean: GitHub didn't allow me to request PR reviews from the following users: FYI.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @alejandrox1 FYI

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.

@alvaroaleman
Copy link
Member

/uncc

I do not have a say in prow.k8s.io config settings 🙃

@k8s-ci-robot k8s-ci-robot removed the request for review from alvaroaleman February 17, 2020 19:31
@droslean
Copy link
Member Author

/assign @stevekuznetsov @fejta

@justaugustus
Copy link
Member

Explicit hold:
/hold

What's the context/justification and was this discussed with the @kubernetes/patch-release-team?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2020
@droslean
Copy link
Member Author

We found ourselves the need to use the cherrypick plugin in kubernetes/kubernetes#88099

Also, there wasn't any discussion with @kubernetes/patch-release-team. We discussed this in the last weekly release meeting but I thought that the plugin was already enabled.

@justaugustus How we should proceed?

@fejta
Copy link
Contributor

fejta commented Feb 18, 2020

Technically looks fine, but can we get some approval from sig-contribex that this is wanted across the org? (Seems useful to me)

@cheftako
Copy link
Member

We found ourselves the need to use the cherrypick plugin in kubernetes/kubernetes#88099

In the short term I ran the cherry-pick script and created kubernetes/kubernetes#88278 for 1.17

@alejandrox1
Copy link
Contributor

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from alejandrox1 February 18, 2020 20:44
@spiffxp
Copy link
Member

spiffxp commented Mar 12, 2020

/cc @justaugustus @cblecker
I think this would be useful to enable for the org, or at least trialing in kubernetes/kubernetes.
It would greatly reduce the friction necessary to cherry-pick a given PR into a release branch

What concerns would you have about enabling this, and what do you think next steps should be?

ref: https://github.com/kubernetes/test-infra/tree/master/prow/external-plugins/cherrypicker

Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiffxp -- The things that were floating around in my head:

  • Did we have a cherrypick plugin before? I feel like we did and turned it off? Is this net new or turning back on the one we removed?
  • What are the failure modes of the plugin?
  • What's the guidance for using this plugin? Is it documented in an easy-to-find place for contributors?
  • How do I edit a CP PR that's not quite a mirror of the parent PR if a bot is creating them?
  • Who creates/manages these cherrypick/XXX labels (that currently don't exist)?

events:
- issue_comment
- pull_request
endpoint: http://cherrypick
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right URL?

@spiffxp
Copy link
Member

spiffxp commented Mar 12, 2020

Did we have a cherrypick plugin before? I feel like we did and turned it off? Is this net new or turning back on the one we removed?

This is net new for kubernetes, though it's been in use for openshift and istio for a while.

I think you're remembering a mungegithub plugin that auto-applied the cherry-pick-approved label when hard-to-understand preconditions were met. We turned that off and never migrated it to prow, prior to migrating kubernetes/kubernetes to tide.

What are the failure modes of the plugin?

You might need to be more specific here. It won't open a PR if one already exists. It will comment on the source PR if a cherrypick failed, and there's discussion about having it file an issue (#16487)

What's the guidance for using this plugin? Is it documented in an easy-to-find place for contributors?

I would suggest we trial to ensure we catch whatever other failure modes you're concerned about, and then update documentation to make this the recommended approach as a followup. If enabled, it would show up in http://go.k8s.io/bot-commands (eg: https://prow.svc.ci.openshift.org/command-help#cherrypick)

How do I edit a CP PR that's not quite a mirror of the parent PR if a bot is creating them?

Here I would recommend closing the PR and opening your own as we do now. The most GitHub could do for us is allow people with repo write access to add new commits to the branch.

Who creates/manages these cherrypick/XXX labels (that currently don't exist)?

My suggestion would be that we trial using this without labels, and solely as a /cherrypick command

/cc @stevekuznetsov @clarketm
is there any additional context you could give from openshift or istio's usage of this plugin?

/hold
FYI @droslean @fejta the cherrypicker service isn't deployed to prow.k8s.io, we would need something equivalent to these files added to prow/cluster/, which should probably done in a separate PR?

@k8s-ci-robot k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 15, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2021
@@ -1081,3 +1081,21 @@ external_plugins:
events:
- issue_comment
- pull_request
kubernetes-sigs/cluster-api:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saschagrunert @justaugustus I think we can remove this from release engineering concern right now given that currently none of these repos where the plugin would be enabled our under or purview. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 13, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 12, 2021
@vincepri
Copy link
Member

Hi folks, what can we do to get this PR merged? Are there any outstanding tasks? @spiffxp @justaugustus

cc @dims

@vincepri
Copy link
Member

vincepri commented Aug 12, 2021

/remove-lifecycle stale

@vincepri
Copy link
Member

/lgtm

for CR and Cluster API

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2021
@dims
Copy link
Member

dims commented Sep 2, 2021

looks like we have sig-release signoff ( https://github.com/kubernetes/test-infra/pull/16329/files#r612485076 )

@dims
Copy link
Member

dims commented Sep 2, 2021

/hold cancel
/approve
/lgtm

we can iterate if needed (token?)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, dims, droslean

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2021
@dims
Copy link
Member

dims commented Sep 2, 2021

/assign @cblecker @spiffxp

Christoph, Aaron,
ok with this? it's been struggling for some time.

@k8s-ci-robot k8s-ci-robot merged commit 0facfb6 into kubernetes:master Sep 2, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 2, 2021
@k8s-ci-robot
Copy link
Contributor

@droslean: Updated the plugins configmap in namespace default at cluster test-infra-trusted using the following files:

  • key plugins.yaml using file config/prow/plugins.yaml

In response to this:

Enable cherrypick plugin for k-sigs/{cluster-api,controller-tools,controller-runtime}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.