Cherrypicks

[guineveresaenger]

Partial attempt to improve contributor experience on cherrypicks; this updates/clarifies some information in cherrypick instructions.

/sig contributor-experience
/sig release
/cc @tpepper @justaugustus @cblecker @spiffxp

reference for tracking work on automation: kubernetes/test-infra#1795

[spiffxp]

nit: link Patch Release Manager to https://github.com/kubernetes/sig-release/blob/master/release-team/README.md#patch-release-manager

[guineveresaenger]

ahhhhh yes this is what I was looking for. Thank you!

[spiffxp]

What does this mean? Someone needs to apply status/approved-for-milestone? Who would that be?

[tpepper]

Is it correct as follows?

* Milestone label must be applied to match the target release branch
* SIG leads associated with the PR must indicate `/status approved-for-milestone`

Or does the milestone labeling happen via the cherry pick script?

[guineveresaenger]

AFAIK, they do not. The two sample cherry-picks I saw did not have them automatically included; they were added manually by the reviewer.
I was going for something like:

Normal rules apply for code merge, such as:

• reviews and lgtms
• milestones and approvals
• passing tests
• hearts and smilies

and then focus on what's different in cherry picks.

[tpepper]

So I feel like this is ok then? It says "Normal rules apply for code merge" and milestone approval requirements are maybe hopefully sort of going to appear in contributors/devel/issues.md in my #2430? But now that I say that here and have posted small updates there, I wonder if that file should not be issues.md but rather something like milestones.md since it's trying to cover issues and PRs?

[nikhita]

SIG leads associated with the PR must indicate /status approved-for-milestone

This is only required during code slush. Otherwise, this label is not really used: https://github.com/kubernetes/kubernetes/pulls?q=is%3Apr+label%3Acherrypick-approved+is%3Aclosed.

[nikhita]

Follow up question: when is cherrypick-candidate used and how is it different from cherrypick-approved? There does not seem to be any consistency in how this label is being used right now.

[guineveresaenger]

I assumed cherrypick-candidate was a nonnecessary pre-stage for cherrypick-approved?

or perhaps it is related to the release cycle?

[guineveresaenger]

I looked for labels in k/k and cherrypick-candidate is obsolete. Out it goes.

[spiffxp]

IMO you can drop "You can find the release team contacts..." if you link Patch Release Manager directly. That description includes a link to all release / patch release branch managers

[tpepper]

Might still want a few words around how the person doing the role changes in each release and to look at the release vX.Y specific table to find the right person.

[guineveresaenger]

@tpepper - thanks to @spiffxp 's link in comment above, I think the flow of information is relatively clear and obvious to infer.

[neolit123]

this section is from 2015 and arguably needs a 2018 refactor outlining the current "norm".
i can summarize, if needed.

[guineveresaenger]

@neolit123 yes, please! I'm not super familiar with the process; I stumbled across it because of a coworker's struggles.

[neolit123]

@guineveresaenger

so here is the review norm from my experience, starting for the cherry-pick idea inception:

• we have an bug/issue which is fixed in a PR for the master branch.
• SIG leads approve the PR and say that this should be cherry picked for past versions.
• we start tracking the issue and PR in an umbrella/tracking issue - e.g.:
  Cherrypick tracking issue for v1.11 kubeadm#958
  (i don't think all projects / sigs do that, also check the small tutorial at the bottom 💯 )
• at this point someone submits a cherry-pick PR and pings the SIG leads
• the SIG leads review the cherry-pick PR, apply the labels[*] and ping the branch manager
• the branch manager sees the cherry-pick PR and given this is approved by SIG leads they can easily approve it as a cherry-pick

ideally the branch manager should not look at commit diffs; approval from SIG leads is required as they understand the code. even in a case where a cherry pick PR is 1to1 with a original PR, there is no guarantee that this change is approved for cherry pick by the SIG leads - the branch manager needs to look for evidence if this is approved for a cherry pick, which can be time consuming. also, in a case of a code conflict while doing the cherry-pick rebase, important code could be removed by the cherry-pick PR creator, so having the SIG eyes on these is kind of a must.

i think that anything outside of that can be considered outside of the norm.
feel free to restructure the above into proper English! :)

[*] what can be streamlined is the amount of labels, SIG leads need to apply to tag a cherry-pick PR, but i think @spiffxp already has some prow proposals for that!

🍒

[guineveresaenger]

@spiffxp @tpepper @neolit123 - PTAL

[neolit123]

SGTM. a couple of GIT specific things that contributors can find difficulties with for the Initiate a Cherry Pick section:

• cherry pick PR does not apply clean against and old release branch and the contributor needs to fix conflicts and rebase. the script does give instructions on how to do that.
• the cherry pick PR includes code that does not pass CI tests. in such a case the contributor has to fetch the auto-generated branch from his fork, amend the problematic commit and force push to the auto-generated branch. (there is also the alternative of creating a new PR, which is noisier).
• the default remote in GIT is called origin not upstream like in the cherry script, this command illustrates better what the user has to do:

FORK_REMOTE=<remote-of-your-fork> UPSTREAM_REMOTE=<remote-of-upstream>
GITHUB_USER=<your-github-user> ./hack/cherry_pick_pull.sh
<remote-of-upstream>/<release-x.xx> <pr-number>

[guineveresaenger]

@neolit123 I'm not entirely sure where to put that information, or if it even belongs here.

Your first two points are true of any PR, if there's conflicts or failing tests it is up to the contributor to fix those. I think it's clear that cherry-pick PRs are no different.
To your last point, if a member is following the recommended github flow, the cherry pick script should run as avertised, but I can see the value of a quick reminder to call one's upstream upstream.

[neolit123]

@guineveresaenger good point.

[nikhita]

👍

[nikhita]

SIG leads' approval is not really required, unless during code slush where the SIG lead will need to apply the status/approved-for-milestone label.

But in general, anyone who has the power to /approve the relevant changes in the PR can approve the cherry-pick.

[guineveresaenger]

Hm, I'm confused. Why do we have the /cherrypick approved label then?

[guineveresaenger]

oh oh oh I see what you're saying - it's not that it takes a SIG lead; approvers have the power of adding the cherry-pick approval. Yes?

[nikhita]

it's not that it takes a SIG lead; approvers have the power of adding the cherry-pick approval. Yes?

Sorry! I should have been clearer.

Cherry pick pull requests have an additional requirement compared to normal pull
requests. They must be approved specifically for cherry-pick by SIG leads.

I meant that cherry-picks don't need any approval from SIG leads.

• To get the approved label, anyone within the approvers list of the relevant OWNERS file can /approve.
• To get the cherrypick-approved label, only the patch release manager can add the label.

[nikhita]

a couple of GIT specific things that contributors can find difficulties with for the Initiate a Cherry Pick section:

I'm not sure if this belongs here but adding to @neolit123's points: I found it really cool that if you run the script again, it will automatically force push to the auto-generated branch. It's really handy because we don't need to create new PRs (which can get messy) if there are any changes. :D

[nikhita]

Also, some good info we can add here would be about cherry-picking to multiple release branches. Ref: #994.

[guineveresaenger]

@nikhita a couple questions:
I'm not sure I entirely understand about re-running the script. I'm assuming you'd run the script with different arguments in that case, as you'd have had to make changes in case of a merge conflict or failing test.

The instructions for milestone approval are really fuzzy - what kind of milestone should be added to cherrypicks? The latest milestone, i.e. currently 1.12?
When we say "oldest branch" does that mean "1.12 is older than 1.11?" This is unclear to me - in terms of existence, 1.12 is older, in terms of creation date, 1.11 is. But maybe this is common parlance that I'm unaware of.

[guineveresaenger]

@neolit123 @nikhita - PTAL
I tried to address the most common concerns and questions. If you could look at it in the light of my questions above to make sure I captured the correct state of things. Thank you!

[nikhita]

I'm not sure if we should include all this info here.

./hack/cherry_pick_pull.sh /<release-x.xx>

This is not needed because an example already exists above.

GITHUB_USER=

This is useful because this is really needed. The script won't work without it. But if you don't set this env var, the script will prompt you to do that. We can include this. 👍

FORK_REMOTE=<remote-of-your-fork> \
   UPSTREAM_REMOTE=<remote-of-upstream> \

Looking at this, it reads to me as I need to set these env vars to get the script to work. This is not true because the script will assume the default fork as "origin" and upstream as "upstream". If someone doesn't have the remotes set this way, only then would they have to set these variables.

Ref: the script has these lines:

UPSTREAM_REMOTE=${UPSTREAM_REMOTE:-upstream}
FORK_REMOTE=${FORK_REMOTE:-origin}

@neolit123 @guineveresaenger thoughts?

[neolit123]

UPSTREAM_REMOTE=${UPSTREAM_REMOTE:-upstream}
FORK_REMOTE=${FORK_REMOTE:-origin}

it depends on the git workflow, i guess.

the steps here suggest cloning upstream:
https://github.com/kubernetes/kubernetes/blob/master/README.md#to-start-developing-kubernetes
git clone https://github.com/kubernetes/kubernetes

this will set origin being the default remote for upstream k/k.

from there users can add their fork as another remote (this is what i do).
so with this workflow the cherry_pick_pull.sh defaults are off and the users need to rename remotes or use the env. variables.

this is the official workflow however:
https://github.com/kubernetes/community/blob/master/contributors/guide/github-workflow.md#2-clone-fork-to-local-storage
it suggests to do:
git clone https://github.com/$user/kubernetes.git
and them setupupstream, which will be compliant with the cherry_pick_pull.sh defaults.

the github suggested workflow is the same:
https://help.github.com/articles/configuring-a-remote-for-a-fork/

so if assume that the users always follow 2. there is no need to set the env.vars.

[guineveresaenger]

whoa, I had no idea there were instructions floating around that effectively set k/k as origin. We should fix that.

[neolit123]

origin as the upstream remote predates github. :)

[nikhita]

Do we need the and rebase in the end? I think fixing conflicts is enough. If we rebase, what do we rebase on top of?

[guineveresaenger]

Yep - too much detail can get confusing. Will take out.

[nikhita]

@guineveresaenger my bad, I don't think this should be included here. :)

What I really meant:
This is can only occur if you created a cherry-pick PR before the parent PR has merged. Then something changed in the parent PR. This change should be reflected in the cherry-pick PR. To do so, you can run the script again, which will pick the changes from the parent PR and force push to the autogenerated branch.

But this is not the ideal way the script should be used, so we should probably not include it here. :)

[neolit123]

@nikhita @guineveresaenger
i think it was added because i made a comment somewhere in this discussion.
but please feel free to remove / improve it if it feels like an edge case.

there is also the case where you create the cherry pick PR using the script, but then for instance CI tests do not pass or something about the code in the old branch is different, and while the diff applies, changes only to the cherry pick PR have to be made.

[guineveresaenger]

yeah those are really special circumstances - I'll take it out.

[nikhita]

I'm not sure I entirely understand about re-running the script. I'm assuming you'd run the script with different arguments in that case, as you'd have had to make changes in case of a merge conflict or failing test.

Added details in #2408 (comment). Sorry about the confusion!

The instructions for milestone approval are really fuzzy - what kind of milestone should be added to cherrypicks?

Each cherrypick PR is against a release branch.

For example, if I'm creating cherry-picks for a fix to backport it to 1.11 and 1.10.

• The cherry-pick PR against the 1.11 branch should get the 1.11 milestone.
• The cherry-pick PR against the 1.10 branch should get the 1.10 milestone.

For example: kubernetes/kubernetes#67393 was a cherry-pick against 1.10, so this got the 1.10 milestone.

[nikhita]

Isn't this only for k/k i.e. not all Kubernetes projects?

[guineveresaenger]

hah, I think that's someone's typo (or otherwise optimism re: other k/repos) and yes. good catch!

[justaugustus]

Milestone requirements seem in need of becoming a bit more rigorous and it looks like they're headed that way, but as they're currently not required I went with an "if you're going to have one, make it match the target release branch".

Do we have an issue to clarify the process surrounding whether something's in a milestone? @tpepper? @justaugustus ?

Not sure that we have docs w.r.t. milestone (at least ones that are easily discoverable). Opened this: kubernetes/sig-release#320

So today something is:

• "in a milestone" (aka "in a release") if it merges to master ahead of release, or is cherry picked
• "in a Milestone" (aka annotated with GitHub Milestone "vX.YY" metadata) if a https://github.com/orgs/kubernetes/teams/kubernetes-milestone-maintainers member has marked it as such manually or via prow "/milestone"

We have an overloaded term.

I wouldn't necessarily call it overloaded.
Milestone, as I'm interpreting it, is "bits that are intended for release x.yy". That is signified by using /milestone vx.yy or manually applying it.

Maybe the problem is that we don't explicitly require it early in the release cycle and we should. I know we bounced this around on a RT or SIG Release call. We should actually enforce it. Opened this for discussion: kubernetes/sig-release#321

If SIG-PM brings forward a process change proposal KEP on feature management, I expect a lot of discussion around it. Right now there's very early draft discussing happening in some google docs. Haven't heard a timeline for solidifying a proposal.

We're rolling on improving the KEP process:

• KEP-1a has merged: https://github.com/kubernetes/community/blob/master/keps/0001a-meta-kep-implementation.md
• Tracking issue: KEP: Enhance the KEP implementation enhancements#617
• Tracking board: https://github.com/orgs/kubernetes/projects/5

[justaugustus]

Thanks for working on this, @guineveresaenger! I know it's been a while since we've had a chance to review.

Tiny nits to address. I'd also s/cherrypick/cherry-pick, as it's how it's referred to in both our labels and the git command.

I dropped some notes below w.r.t milestones as well: #2408 (comment)

[justaugustus]

s/e.g.milestone/e.g., milestone

[AishSundar]

A couple of ques (my apologies if these have been discussed before)

1. For a new upcoming minor release, e.g: 1.13, who actually applies the 'cherrypick-approved' label, the PatchRelease manager or the branch manager for the release?

2. If a PR is to be CP'ed into 2 branches, what milestone should be applied to the PR?

[AishSundar]

I found out the answer for my ques no.2 by reading through the conversations. We essentially will have a CP PR for each of the target release branch. So the milestone on each of the PR will correspond to the release into which it will be CP'ed.

[guineveresaenger]

regarding your first question, AFAIK the patch release manager applies the cherrypick-approved label.

[guineveresaenger]

oh I see your question now - sorry! 😓 I'm honestly not sure, will ask.

[nikhita]

AFAIK the patch release manager applies the cherrypick-approved label.

Yes, this is correct. :)

The patch release manager is responsible for approving cherrypicks.

[justaugustus]

This dashboard doesn't load now, probably because the cherrypick-candidate label no longer exists. Filed this to fix / remove: kubernetes/k8s.io#140

[AishSundar]

if this dashboard is removed (or isnt functional), is the only way to review pending PRs for CP is via GH query. It might be useful to have a sample query here.

[guineveresaenger]

Thank you @justaugustus! I’m behind due to new environment setup and CLA transfer but thank you so much for the review. Will follow up ASAP.

[guineveresaenger]

@justaugustus - I swear, it used to say "cherrypick"! I triple checked!

[spiffxp]

You have this statement on a new line, but markdown just joins it to the previous bullet. Which looks fine to me, just wondering if that was your intent

[guineveresaenger]

Really!? That's super strange actually I do not see thatat all!?

[spiffxp]

/approve
/hold
I'm totally fine with the content as-is. It'd be nice if this got word-wrapped to 80 chars but I will by no means block on that. If any of the previous reviewers are fine with this as-is, feel free to /lgtm and /hold cancel

[nikhita]

This gives a 404 due to git.k8s.io. We need to use the complete github.com url (like it's done for do-not-merge/cherry-pick-not-approved below).

[nikhita]

I think this needs a bullet point.

[justaugustus]

Yep. This needs bullet points.
Also, the first query should be: https://github.com/kubernetes/kubernetes/pulls?utf8=%E2%9C%93&q=is%3Aopen+is%3Apr+label%3Acherry-pick-approved

[nikhita]

A couple of nits, but lgtm from my side. 👍

[guineveresaenger]

@nikhita -PTAL

[guineveresaenger]

actually, hold a sec, wanted to get some clarification on branch mgr vs. patch release mgr

[justaugustus]

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justaugustus, spiffxp

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:

• contributors/devel/OWNERS [spiffxp]

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