doc: add guide for backporting PRs #11099

evanlucas · 2017-02-01T12:00:43Z

This guide should help answer questions for contributors
that are not familiar with the backport process.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

addaleax · 2017-02-01T12:05:17Z

doc/guides/backporting-to-release-lines.md

+
+Each release line has a staging branch that the releaser will use a scratch pad
+while preparing a release. The branch name is formatted as follows:
+`vn.x-staging` where `n` is the major release number.


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

gibfahn · 2017-02-01T12:27:56Z

Does this cover the same ground as #10343?

If so I guess we should decide whether we want this as a separate guide linked to from the COLLABORATOR_GUIDE (i.e. this PR), or just a note at the bottom of the COLLABORATOR_GUIDE #10343.

I'm undecided.

evanlucas · 2017-02-01T12:49:51Z

@gibfahn oh wow, thanks. I totally missed that one

jasnell · 2017-02-01T16:46:35Z

There's likely value in separating it out

sam-github

Great to see this documented so the process can become more regular.

sam-github · 2017-02-01T18:19:28Z

doc/guides/backporting-to-release-lines.md

+
+## Staging branches
+
+Each release line has a staging branch that the releaser will use a scratch pad


"as a scratch"

sam-github · 2017-02-01T18:21:25Z

doc/guides/backporting-to-release-lines.md

+If a cherry-pick does not landing cleanly on the staging branch, the releaser
+will mark the pull request with a particular label for that release line,
+specifying to our tooling that this pull request should not be included. The
+releaser will then add a comment that a backport is needed.


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.

sam-github · 2017-02-01T18:22:11Z

doc/guides/backporting-to-release-lines.md

+## What needs to be backported?
+
+When a release is being prepared, the releaser attempts to cherry-pick a
+certain set of commits from the master branch to the release staging branch.


"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.

sam-github · 2017-02-01T18:22:56Z

doc/guides/backporting-to-release-lines.md

+
+## What can be backported?
+
+The "Current" release line (currently v7.x) is much more lenient in what can


lenient than what?

shouldn't semver-major be mentioned here somewhere?

sam-github · 2017-02-01T18:25:26Z

doc/guides/backporting-to-release-lines.md

+
+## How to submit a backport pull request?
+
+For these steps, let's assume that a backport is needed for the v7.x release


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.

sam-github · 2017-02-01T18:28:16Z

doc/guides/backporting-to-release-lines.md

+git checkout -b backport-10157
+```
+
+*Note: the branch name does not have to have the pull request number in it.*


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

I like it. Will update

sam-github · 2017-02-01T18:29:32Z

doc/guides/backporting-to-release-lines.md

+* After creating the branch, apply the changes to the branch.
+* The commit message should be identical to the commit message on the master
+  branch, unless the commit has to be different due to dependencies that are
+  not present in the targeted release line.


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).

sam-github · 2017-02-01T18:30:31Z

doc/guides/backporting-to-release-lines.md

+### Helpful Hints
+
+* Please include the backport target in the pull request title.
+  * Ex. `process: improve performance of nextTick (v4.x backport)`


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

sam-github · 2017-02-01T18:33:56Z

doc/guides/backporting-to-release-lines.md

+A good example of a commit that has been backported is
+https://github.com/nodejs/node/commit/2d23562588eebaf9a2a3d3987fe6e4ef63ab9500.
+
+* Original Pull Request: https://github.com/nodejs/node/pull/10157


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

gibfahn · 2017-02-01T20:14:44Z

doc/guides/backporting-to-release-lines.md

+
+When a release is being prepared, the releaser attempts to cherry-pick a
+certain set of commits from the master branch to the release staging branch.
+If a cherry-pick does not landing cleanly on the staging branch, the releaser


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>.

gibfahn · 2017-02-01T20:17:52Z

doc/guides/backporting-to-release-lines.md

+If a cherry-pick does not landing cleanly on the staging branch, the releaser
+will mark the pull request with a particular label for that release line,
+specifying to our tooling that this pull request should not be included. The
+releaser will then add a comment that a backport is needed.


Related: nodejs/github-bot#120

gibfahn · 2017-02-01T20:21:20Z

doc/guides/backporting-to-release-lines.md

+# Assuming your fork of Node.js is checked out in $NODE_DIR,
+# the origin remote points to your fork, and the upstream remote points
+# to git://github.com/nodejs/node
+cd $NODE_DIR


Might be easier to just say something like:

git clone -o upstream https://github.com/nodejs/node.git && cd node git remote add fork [email protected]:$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.

gibfahn · 2017-02-01T20:23:13Z

doc/guides/backporting-to-release-lines.md

+
+*Note: the branch name does not have to have the pull request number in it.*
+
+* After creating the branch, apply the changes to the branch.


Maybe specify cherry-picking the commit across?

git cherry-pick $SHA # Use your commit hash

gibfahn · 2017-02-01T20:24:32Z

doc/guides/backporting-to-release-lines.md

+cherry-picked cleanly from master, then nothing needs to be done. If not, a
+backport pull request will need to be submitted.
+
+## How to submit a backport pull request?


Don't need the ?

gibfahn · 2017-02-01T20:26:01Z

We should definitely link to this from the COLLABORATOR_GUIDE.

This guide should help answer questions for contributors that are not familiar with the backport process.

evanlucas · 2017-02-03T01:42:19Z

Ok, updated. PTAL. I went ahead and added to the COLLABORATOR_GUIDE as well. I removed an example commit since I (unfortunately) couldn't find one that actually used the backport PR-URL instead of the original. Thanks!

mhdawson · 2017-02-03T22:06:16Z

doc/guides/backporting-to-release-lines.md

+```shell
+git cherry-pick $SHA # Use your commit hash
+```
+


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.

joyeecheung · 2017-02-04T11:40:33Z

doc/guides/backporting-to-release-lines.md

+```
+
+* The commit message should be as close as possible to the commit message on the
+  master branch, unless the commit has to be different due to dependencies that


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.

sam-github · 2017-02-11T22:53:56Z

Maybe we should have explicit meta-data on backport commits, like Backport: <URL to PR being backported>? Edit: which I realize from email notifications that @evanlucas already suggested, but the comment is hidden above.

gibfahn · 2017-02-12T00:14:34Z

Having another line seems like a good idea, so @sam-github are you suggesting that if we have:

PR-URL: url1
Backport: url2

then PR-URL: url1 is the original PR, and Backport: url2 is the vN.x Backport PR (not the other way around)? I guess it might make sense to keep PR-URL: the same across backports.

evanlucas · 2017-02-13T14:03:30Z

@sam-github @gibfahn I agree. I went ahead and added it in. PTAL. Thanks!

targos · 2017-02-13T14:11:08Z

In the metadata of the backport commit, who should be displayed as Reviewed-By?

Reviewers of the original PR?
Reviewers of the backport PR?
Both?

evanlucas · 2017-02-13T14:12:25Z

@targos good point. IMO, if a backport PR is necessary, it should go through the standard process of reviewers. I think the reviewers from the backport PR should be listed, not the reviewers from the original PR.

sam-github · 2017-02-13T20:47:50Z

@gibfahn I think PR-URL should point to the PR where this specific commit was reviewed, and the meta-data should refer to the specific (backport) PR.

So, something like this:

The original PR would be:

commit message

PR-URL: original PR (as it is now)
... (metadata as it is now)

then it gets backported and PRed, a new PR (obviously):

commit message

PR-URL: the URL for the PR where the backport was proposed and discussed
Backport-of: the URL to the original PR that needed to be backported
... metadata of *the backport* (not a copy of the original)

I changed Backport to Backport-of because your question made me realize calling the original PR a "backport" is strange and confusing.

If its way too much work, maybe it can't happen, but landing backports, which are rewritten PRs, with metadata claiming they have been reviewed by people who may not in fact have reviewed the rewrite seems misleading.

MylesBorins · 2017-02-13T21:29:19Z

We have so far been using the original PR for the PR-URL and including the backport PR as a Refs:

This has not been consistent though. I'm up for whatever re: labelling as long as it is consistent and documented

gibfahn · 2017-02-14T00:08:44Z

I changed Backport to Backport-of because your question made me realize calling the original PR a "backport" is strange and confusing.

@sam-github yeah, that was my issue. I'm fine with using Backport-of: for the original PR to master, it makes sense that `PR-URL:`` should always point to the actual PR that added this specific commit.

The Reviewed-By: field is (in my experience) already redone to only contain the reviewers of the backport PR.

gibfahn

I think we should clarify what the 2 week rule applies to.

gibfahn · 2017-02-16T04:19:23Z

doc/guides/backporting-to-release-lines.md

+is reviewed. When each commit is landed, the new reviewers and the new PR-URL
+should be used.
+
+[`LTS Plan`]: https://github.com/nodejs/LTS#lts-plan


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

gibfahn · 2017-02-16T04:21:49Z

doc/guides/backporting-to-release-lines.md

+The "Current" release line (currently v7.x) is much more lenient than the LTS
+release lines in what can be landed. Our LTS release lines
+(currently v4.x and v6.x) require that commits live in a Current release for at
+least 2 weeks before backporting. Please see the [`LTS Plan`][] for more


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.

gibfahn · 2017-02-16T04:27:46Z

I also had another question, if @nodejs/lts decides what gets backported to LTS, and @nodejs/release do the releases, who decides what gets backported to current?

jasnell · 2017-02-16T18:29:40Z

Essentially anything sitting in master that is not semver-major or explicitly marked dont-land for the current should land in current. The person doing the release selects the set. If a PR needs to be backported, then either the release person will do it or will ask someone to do it, otherwise it won't make it in.

MylesBorins

Comment inline about one inaccuracy with current process

MylesBorins · 2017-03-27T20:07:08Z

doc/guides/backporting-to-release-lines.md

+
+Switch to using String#repeat to improve performance.
+
+PR-URL: https://github.com/nodejs/node/pull/5678


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

MylesBorins

LGTM

evanlucas · 2017-03-27T21:19:15Z

@rvagg mind taking a quick look? Thanks!

targos · 2017-03-29T08:40:21Z

doc/guides/backporting-to-release-lines.md

+
+Switch to using String#repeat to improve performance.
+
+PR-URL: https://github.com/nodejs/node/pull/1234


@targos see #11099 (comment)

FWIW looking at backport PRs they seem to vary.

evanlucas · 2017-04-10T21:02:08Z

ping @rvagg

Following on from discussion in: nodejs/Release#191 the backport PR should start with the same commit message. Any additions can be made by whoever lands the PR.

gibfahn · 2017-04-12T18:49:50Z

In the backporting meeting we decided that it would be easiest (at least for now) if people raising Backport PRs didn't have to modify the commit message, so I've added a commit to remove that info.

cc/ @MylesBorins @sam-github @addaleax @mhdawson

mhdawson

Still LGTM from me.

Issues have all been fixed.

gibfahn · 2017-04-13T16:18:13Z

Does anyone have any objection to this landing? Any further issues can be looked at later, and it would be really good to have something to refer people to.

MylesBorins

tiny nit: LGTM

MylesBorins · 2017-04-13T18:05:59Z

doc/guides/backporting-to-release-lines.md

+## What needs to be backported?
+
+If a cherry-pick from master does not land cleanly on a staging branch, the
+releaser will mark the pull request with a particular label for that release


@gibfahn should we specify the label?

sam-github · 2017-04-13T18:47:08Z

LGTM, still. Get it in, iterate on it as we go.

gibfahn · 2017-04-16T13:43:24Z

Landed in ece9ab5

This guide should help answer questions for contributors that are not familiar with the backport process. PR-URL: #11099 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This guide should help answer questions for contributors that are not familiar with the backport process. PR-URL: nodejs/node#11099 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 1, 2017

addaleax approved these changes Feb 1, 2017

View reviewed changes

mscdex added the meta Issues and PRs related to the general management of the project. label Feb 1, 2017

sam-github suggested changes Feb 1, 2017

View reviewed changes

gibfahn reviewed Feb 1, 2017

View reviewed changes

doc: add guide for backporting prs

f153855

This guide should help answer questions for contributors that are not familiar with the backport process.

evanlucas force-pushed the backportguide branch from 9425052 to f153855 Compare February 3, 2017 01:40

mhdawson reviewed Feb 3, 2017

View reviewed changes

joyeecheung reviewed Feb 4, 2017

View reviewed changes

evanlucas added 2 commits February 4, 2017 09:13

fixup! commit message

837bcb9

fixup! cherry-pick instructions

aa40433

fixup add backport metadata line

444359c

fixup! add details regarding Backport-of

066e617

gibfahn mentioned this pull request Feb 16, 2017

Backport test abort on timeout to v4.x #11351

Closed

2 tasks

gibfahn suggested changes Feb 16, 2017

View reviewed changes

MylesBorins suggested changes Mar 27, 2017

View reviewed changes

fixup! use original PR-URL

fc434b2

MylesBorins approved these changes Mar 27, 2017

View reviewed changes

targos reviewed Mar 29, 2017

View reviewed changes

refack changed the title ~~doc: add guide for backporting prs~~ doc: add guide for backporting PRs Apr 10, 2017

squash!: remove commit message changes

a374208

Following on from discussion in: nodejs/Release#191 the backport PR should start with the same commit message. Any additions can be made by whoever lands the PR.

gibfahn mentioned this pull request Apr 12, 2017

Backporting Team Meeting, April 12th 2017 nodejs/Release#191

Closed

mhdawson approved these changes Apr 13, 2017

View reviewed changes

MylesBorins approved these changes Apr 13, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

gibfahn closed this Apr 16, 2017

gibfahn mentioned this pull request Apr 16, 2017

doc: describe labelling process for backports #12431

Merged

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the land-on-v6.x label May 15, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged


		## Staging branches

		Each release line has a staging branch that the releaser will use a scratch pad


		## What can be backported?

		The "Current" release line (currently v7.x) is much more lenient in what can


		## How to submit a backport pull request?

		For these steps, let's assume that a backport is needed for the v7.x release


		Note: the branch name does not have to have the pull request number in it.

		* After creating the branch, apply the changes to the branch.


		Switch to using String#repeat to improve performance.

		PR-URL: https://github.com/nodejs/node/pull/5678

doc: add guide for backporting PRs #11099

doc: add guide for backporting PRs #11099

Conversation

evanlucas commented Feb 1, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

gibfahn commented Feb 1, 2017

evanlucas commented Feb 1, 2017

jasnell commented Feb 1, 2017

sam-github 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

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

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

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

gibfahn Feb 1, 2017 • 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn commented Feb 1, 2017

evanlucas commented Feb 3, 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

sam-github commented Feb 11, 2017 • edited Loading

gibfahn commented Feb 12, 2017

evanlucas commented Feb 13, 2017

targos commented Feb 13, 2017

evanlucas commented Feb 13, 2017

sam-github commented Feb 13, 2017

MylesBorins commented Feb 13, 2017

gibfahn commented Feb 14, 2017

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn Feb 16, 2017 • 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

gibfahn commented Feb 16, 2017

jasnell commented Feb 16, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins left a comment

Choose a reason for hiding this comment

evanlucas commented Mar 27, 2017

Choose a reason for hiding this comment

gibfahn Feb 1, 2017 •

edited

Loading

sam-github commented Feb 11, 2017 •

edited

Loading

gibfahn Feb 16, 2017 •

edited

Loading

gibfahn Mar 29, 2017 •

edited

Loading