Just a suggestion but how about a capital N? vN.x-staging might be a bit clearer.

"as a scratch"

what is the label? the labelelling has to distinguish things that should be backported (but aren't landing cleanly) from things that should not be backported (clean cherry-pick or not)

Related: nodejs/github-bot#120

Yea I wasn't quite sure how to word it being as the label is different for each release line.

I agree that we should have 1 label for something that doesn't land and another for 1 that shouldn't land. That being said, I think the reasoning for not having them is that we should land everything in master that isn't a breaking change and a breaking change will be marked as semver major.

"certain" is vague, but elaborated on below, maybe state that criteria for consideration depends on the the target version (current or LTS)?

actually, not elaborated on below, I don't think.

Yea, I hear ya. It is a bit difficult to find the middle ground between spilling my guts on cutting releases vs the actual intention of the guide. I'll improve this.

I think ideally this would link to the part of releases.md which talks about the criteria, not sure if there is currently anything in there to link to though.

There is not anything in the releases.md document regarding this. I'll work on documenting that more specifically as well.

I don't think that level of information belongs in this doc, if we can actually figure out what "certain" means then it belongs elsewhere anyway.

lenient than what?

shouldn't semver-major be mentioned here somewhere?

This is unusual, isn't it? that a backport is needed from master to Current? Shouldn't master always pick clean to Current?

not necessarily. Master takes semver-major changes. I see a lot of conflicts from changes that go in after a major change so they have to be backported quite a bit in that case.

Recommending a consistent branch naming is a good thing: backport-<PR#>-to-<target node version>

I like it. Will update

what do you mean by "deps not present"? If its a manual backport, the code is changing, sometimes a lot, should the commit message still be exactly the same?

That's a fair point. I guess the answer in that case would be "it depends". I don't think we should have identical commit messages if they are incorrect, but I do think we need a better way to show, hey this is a backport commit (like the v8 backports).

Instead of an example, I suggest we be prescriptive. I've been doing "v4.x backport: proces: improve...", I thought that was requested, but I'm happy to do above instead.

I'd rather we put "v4.x backport" at the beginning; it makes it stand out more.

+1 for specifying exactly.

that works for me. I added this so we could actually come up with a specified way that we request (and can edit to make sure is followed). I would rather we be opinionated on this. It makes looking for backports easier IO

neither the commit above, nor the PR it links to, has these two lines in the commit message or PR description, can you point to where they should be? In the commit message when it lands, the backport description?

Maybe our process should add another PR-URL-like piece of metadata for commits that had to be reworked and PRed as a backport because they didn't land clean? The current metadata seems like a bit of a lie, the reviewers never saw the particular code diff that is landing in the backport.

I thought that the procedure was to get LGTMs on the backport PR, and include those (not the LGTMs from the original PR) when landing.

I do think we need to come up with an additional piece of metadata for the original PR-URL. Part of the reason for opening this is so we can get to the point where everyone knows how to handle these, because right now, things are kind of all over the place. +1 for including the reviewers from the backport PR vs the original PR

landing->land

Also, do we do three-way merging (git am -3)? If so it might be worth specifying that.

no, we only cherry-pick (or I only cherry-pick). /cc @nodejs/release do any of yall actually fixup conflicts when cherry-picking?

I'm pretty sure I got this (in my ~/.gitconfig) from @MylesBorins :

  pa = "!curl -L $1.patch | git am --whitespace=fix #" # Patch Github PR
  pap = "!curl -L $1.patch | git am -3 --whitespace=fix #" # Please patch it

Yes, when landing a commit for pushing to master, we use git am. For backporting, the goal is to get the commit unmodified, so we use git cherry-pick <sha>.

Related: nodejs/github-bot#120

Might be easier to just say something like:

git clone -o upstream https://github.com/nodejs/node.git && cd node
git remote add fork git@github.com:$USER/node.git
git fetch --all

I'd find it easier to understand, and it doubles as a guide for someone not sure about setting up multiple remotes.

sure, although I would think that origin is more common than fork

I like to be specific in instructions, origin can end up being upstream or fork.

I guess I'm a lot more used to https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-1-fork

Okay fair enough, makes sense to keep it consistent then.

Maybe specify cherry-picking the commit across?

git cherry-pick $SHA # Use your commit hash

Don't need the ?

At this point do we need to say what you should expect to see and how to fix it up? I assume that following this guide is only needed if the cherry-pick is going to fail (I'm also assuming that $SHA is for the original commit that went into master)

I'll clarify. Yes, this guide is for when the releaser cannot cleanly cherry-pick the commit and requests a backport.

If I understand correctly, this means the metadata(PR urls, reviewers, etc) should remain the same as the original commit?

In the case of a backport, I don't think it should. I'll clarify that.

Is there a reason you used [`LTS Plan`] rather than: [LTS Plan]? It looks a bit odd to me.

As I understand it, the two week rule doesn't apply to doc fixes and infrastructure changes (things that don't change what we ship). This is probably a question for @nodejs/lts, but either way we should probably be explicit here.

Refs: #11351 (comment) and nodejs/build#613 (comment)

cc/ @bnoordhuis @MylesBorins @misterdjules

The length of time is not set in stone as is based on the potential impact. A doc change has no impact on runtime stability so those, as long as they are relevant to the version, can pretty much land any time. A code change, however, should go through at least one current release cycle to ensure that there are no regressions reported. For changes to more sensitive parts of the code, the length of time may be longer. Anything that is semver-minor will typically be batched up and held much longer than semver-patch so that we have less frequency between LTS minors.

Based on what James said, I think we should remove this part. How does one know what sensitive parts of the codebase are when submitting a backport PR? It's hard to document something that isn't concrete imo.

It's definitely a grey area, but people do talk about the two week rule, so I think we should try and document it somehow, even if we have to say that it's quite subjective.

This is not accurate. we maintain the original PR-URL

5678

@targos see #11099 (comment)

FWIW looking at backport PRs they seem to vary.

@gibfahn should we specify the label?