doc: Update CONTRIBUTING.md with backport info #10343

sxa · 2016-12-19T19:22:34Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Description of change

Add information on getting fixes into the LTS releases, and also
some information on what is needed for getting fixes to dependencies
back into the Node.js codebase.

While the top level CONTRIBUTING.md is great for people contributing into the master branch, it's not so clear on the process for getting changes into the current and LTS releases. This PR has some proposed additions to describe the process that contributors should know about, and what they may have to do, to get their fixes back into the LTS releases after they have got them into master.

richardlau · 2016-12-19T19:32:58Z

CONTRIBUTING.md

+
+Items may be selected as candidates for inclusion into the "active LTS"
+branch(es), especially if they are bug fixes.  This will be at the
+discretion of someone the person releasing that new version by default, but


Extra "someone" here.

Fixed - cheers

sam-github · 2016-12-19T19:46:23Z

CONTRIBUTING.md

+its-watch-v6.x) and adding a suitable comment.  If the changes can just be
+cherry picked across without merge conflicts this will often be done by a
+member of the release team, but if not then they may request a manual merge,
+for which a pull request will be required.


I think the dont-land-on-XXX deserves mention, as the opposite of lts-watch

A fair point, although I'm not sure how often a non-collaborator would choose to add it. Could be useful for them to know about it if it's added to one of their PRs though. Done.

gibfahn · 2016-12-20T07:37:58Z

CONTRIBUTING.md

+Items may be selected as candidates for inclusion into the "active LTS"
+branch(es), especially if they are bug fixes.  This will be at the
+discretion of the person releasing that new version by default, but
+anyone can request a backport by adding the label "its-watch-vX.x" (e.g. 


Well, anyone can request that a collaborator add the lts-watch-vX.x or dont-land-on-vX.x labels by adding a suitable comment.

Also "its-watch-vX.x" -> lts-watch-vX.x ( i -> l and use backticks) for consistency.

We should also probably link to https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#how-can-i-help

gibfahn · 2016-12-20T07:40:55Z

General idea makes sense to me, but we should be careful about duplicating information about LTS dates and backporting guides from the LTS repo and the LTS section on the Collaborator guide.

EDIT: I suspect the answer is to move some of the Collaborator guide info into this document.

jasnell · 2016-12-23T06:49:56Z

/cc @thealphanerd

MylesBorins

I really appreciate the work that has gone into this, and it is important information to document.

My primary concern with the current iteration is that it is a bit verbose. I've made some inline suggestions of how to substantially trim down the copy here.

Please let me know if you think that my suggestions are going to over simplify things.

MylesBorins · 2016-12-23T17:04:47Z

CONTRIBUTING.md

+member of the release team, but if not then they may request a manual merge,
+for which a pull request will be required.  Also related to the lts-watch-
+labels, if you want to ensure something is not backported, label it with
+dont-land-on-vX.x (e.g.  dont-land-on-v6.x)


I find the above paragraph a bit verbose and not entirely accurate. Perhaps consider

Any change can be considered for LTS by applying the appropriate lts-watch-vX.x label for the branch you would like it backported to. If a commit is not to be considered for backporting please apply the dont-land-on-vX.x label. If it can be cherry-picked cleanly it will be done by someone from the release team, if a manual backport is required you may be asked to submit a PR against the staging branch of the release line the backport is targetting.

Yep that wording works for me too and I don't think it is oversimplifying. Have adjusted accordingly.

MylesBorins · 2016-12-23T17:06:26Z

CONTRIBUTING.md

+details of each are on the LTS schedule page.  Once Pull Requests have
+landed in master they will often automatically become candidates for the
+next version of the "current" release, in which case the process mentioned
+above regarding cherry picking and potentially pull requests will apply.


While the information regarding the current release is important, is this the best place to have it? It is mentioned above that commits should live in current for at least two weeks. I think we can remove this paragraph

I get where you're coming from, but do we explicitly document it anywhere else that can be referenced? There is the diagram on the LTS page, but there are almost no other references to what "current" means (as distinct from LTS) anywhere else in the document (and the section in the LTS doc on NaN currently has a reference to "current LTS" which possibly doesn't help - I'll submit a change to make that "current and Active LTS")

The LTS page says "New semver-major releases of Node.js are cut from master every six months." but it doesn't specifically call that out as being the definition of "current". I could add the word current in a few places in the LTS document if we choose to drop it from CONTRIBUTING.md if that would be preferable, but I'm not too keen on just dropping it at the moment.

+1 to documenting this in the LTS page and then linking to it here, in which case I assume you could collapse the LTS Contributions and Current Release sections into one Backporting section.

Updated to remove the section on the basis that nodejs/Release#174 lands, clarifying what "current" means.

MylesBorins · 2016-12-23T17:14:02Z

CONTRIBUTING.md

+```
+
+Any other relevant links to pull requests can be included as "Ref:" links in
+the PR message


I'm not sure that this needs to be an entire section. The abridged version of the top paragraph I suggested could likely be supplemented to include the additional information found here

A manually backported change should include the same set of commits that originally landed on master, this includes all meta data. The easiest way to do this is to use git cherry-pick

$ git cherry-pick SHA-OF-ORIGINAL-COMMIT // fix all conflicts that are reported $ git cherry-pick --continue

Yep I'm good with that. Done.

gibfahn · 2016-12-28T14:38:57Z

@sxa555 If you rebase against master, it's probably worth putting the extra info in the Additional Notes section (it didn't exist when you opened this PR).

Add information on getting fixes into the LTS releases, and also some information on what is needed for getting fixes to dependencies back into the Node.js codebase. doc: Separate out current release from LTS in CONTRIBUTING.md doc: Add info on bumping V8 path level

sxa · 2016-12-30T13:50:29Z

CONTRIBUTING.md

+familiar with the LTS information and schedules at
+https://github.com/nodejs/LTS#lts-schedule
+
+Items which have already landed on master will be candidates for the


@thealphanerd Are you ok with "Items" at the start of this paragraph? I'm thinking I'd prefer "commits" or "Pull Requests" now.

Changed to "commits"

gibfahn

Various nits

gibfahn · 2017-01-06T22:23:56Z

CONTRIBUTING.md

+
+Before backporting or contributing to the LTS releases make sure you are
+familiar with the LTS information and schedules at
+https://github.com/nodejs/LTS#lts-schedule


LTS information and schedules at https://github.com/nodejs/LTS#lts-schedule -> [LTS information and schedules](https://github.com/nodejs/LTS#lts-schedule).

gibfahn · 2017-01-06T22:25:14Z

CONTRIBUTING.md

+For developing new features and bug fixes, the `master` branch should be
+pulled and built upon.  See the section on
+[backporting](#backporting-to-current-and-lts-branches) if you would like
+the fix to be backported.


if you would like the fix to be backported.

I'm thinking that new contributors probably have no idea if they'd like something to be backported, maybe something like:
if you think the fix should be applied to current release lines (or something).

gibfahn · 2017-01-06T22:29:14Z

CONTRIBUTING.md

+Commits which have already landed on master will be candidates for the
+"current" release branch(es) and can then also become candidates for
+inclusion into the "Active LTS" branch(es), especially if they are bug
+fixes.  Any change can be considered for LTS by requesting @nodejs/lts apply


@nodejs/lts -> a collaborator

I wanted it to be a bit more specific bearing in mind this should be a fairly self-contained document for beginners. I could point to a list of collaborators and say "Pick a random one from this list" but I would think that this would be a more useful default action for newbies.

gibfahn · 2017-01-06T22:29:40Z

CONTRIBUTING.md

+"current" release branch(es) and can then also become candidates for
+inclusion into the "Active LTS" branch(es), especially if they are bug
+fixes.  Any change can be considered for LTS by requesting @nodejs/lts apply
+the appropriate lts-watch-vX.x label for the branch you would like it


needs backticks:

lts-watch-vX.x -> `lts-watch-vX.x`

gibfahn · 2017-01-06T22:29:46Z

CONTRIBUTING.md

+fixes.  Any change can be considered for LTS by requesting @nodejs/lts apply
+the appropriate lts-watch-vX.x label for the branch you would like it
+backported to.  If a commit is not to be considered for backporting please
+ask for it to be labelled with dont-land-on-vX.x.  If it is to be backported


gibfahn · 2017-01-06T22:31:52Z

CONTRIBUTING.md

+backported to.  If a commit is not to be considered for backporting please
+ask for it to be labelled with dont-land-on-vX.x.  If it is to be backported
+and can be cherry-picked cleanly it will be done by someone from the release
+team.  Generally all changes should have landed in the current release for


@MylesBorins Should all changes should be qualified, maybe all significant changes or all functional changes?

gibfahn · 2017-01-06T22:32:26Z

CONTRIBUTING.md

+
+If a manual backport is required when porting your change to the current or
+LTS branches you may be asked to submit a PR against the staging branch of
+the release line the backport is targetting (vX.x-staging).  The easiest way


gibfahn · 2017-01-06T22:52:50Z

CONTRIBUTING.md

+```
+
+If you need to reference other relevant pull requests regarding the
+conflicts, list them as "Ref:" links in the PR message.


Amazingly there's no standard for using Ref: vs Refs: (and it's not mentioned anywhere). I feel we should pick one and document it.

EDIT: #10670

s/Ref:/Refs:/

I'd already made that change but apparently hadn't pushed it. I have now.

gibfahn · 2017-01-19T13:03:04Z

ping @sxa555

gibfahn

LGTM, thanks for adding the line about including "vX.x backport in the PR title".

gibfahn · 2017-01-19T14:09:57Z

@sam-github @richardlau @MylesBorins any other issues?

I guess also cc/ @nodejs/lts

jasnell · 2017-01-19T17:06:29Z

CONTRIBUTING.md

-For developing new features and bug fixes, the `master` branch should be pulled
-and built upon.
+For developing new features and bug fixes, the `master` branch should be
+pulled and built upon.  See the section on


The use of "should" in this first sentence has always bothered me. The master branch is the branch for new features and bug fixes, not should be. So perhaps changing this to:

New features and bug fixes are landed in `master` first, then backported to release branches. See the section on [backporting]...

jasnell · 2017-01-19T17:08:00Z

CONTRIBUTING.md

+```
+   $ git cherry-pick HASH
+   // fix all conflicts that are reported
+   $ git cherry-pick --continue


might be worthwhile mentioning that git add {fixed-file} is needed before continuing

jasnell · 2017-03-24T23:05:22Z

Is this superseded by #11099?

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

@jasnell I believe so

TimothyGu · 2017-04-22T23:41:59Z

Closing as it is superseded by #11099.

nodejs-github-bot added doc Issues and PRs related to the documentations. lts-watch-v6.x labels Dec 19, 2016

richardlau reviewed Dec 19, 2016

View reviewed changes

sxa force-pushed the CONTRIBUTING.lts branch from 7d1f25a to 66be1fb Compare December 19, 2016 19:38

nodejs-github-bot added lts-watch-v6.x labels Dec 19, 2016

sam-github reviewed Dec 19, 2016

View reviewed changes

gibfahn reviewed Dec 20, 2016

View reviewed changes

sxa force-pushed the CONTRIBUTING.lts branch from 66be1fb to 7ce5e14 Compare December 20, 2016 11:31

nodejs-github-bot added lts-watch-v6.x labels Dec 20, 2016

gibfahn self-assigned this Dec 23, 2016

MylesBorins suggested changes Dec 23, 2016

View reviewed changes

nodejs-github-bot added lts-watch-v6.x labels Dec 28, 2016

sxa force-pushed the CONTRIBUTING.lts branch from 3aca0b9 to 392b3e7 Compare December 28, 2016 14:00

nodejs-github-bot added lts-watch-v6.x labels Dec 28, 2016

sxa force-pushed the CONTRIBUTING.lts branch from 392b3e7 to c709e16 Compare December 29, 2016 15:03

nodejs-github-bot added lts-watch-v6.x labels Dec 29, 2016

sxa force-pushed the CONTRIBUTING.lts branch from c709e16 to 26e81b6 Compare December 29, 2016 15:09

nodejs-github-bot added lts-watch-v6.x labels Dec 29, 2016

sxa force-pushed the CONTRIBUTING.lts branch from 26e81b6 to 69316ce Compare December 29, 2016 15:12

nodejs-github-bot added the dont-land-on-v7.x label Dec 30, 2016

sxa force-pushed the CONTRIBUTING.lts branch from 0da89c2 to 30a5e4b Compare December 30, 2016 13:46

nodejs-github-bot added the dont-land-on-v7.x label Dec 30, 2016

sxa commented Dec 30, 2016

View reviewed changes

sxa force-pushed the CONTRIBUTING.lts branch from 30a5e4b to f2d641e Compare January 3, 2017 18:09

nodejs-github-bot added the dont-land-on-v7.x label Jan 3, 2017

gibfahn suggested changes Jan 6, 2017

View reviewed changes

nmschulte mentioned this pull request Jan 12, 2017

Request: support out of tree build #10748

Closed

mscdex removed the dont-land-on-v7.x label Jan 13, 2017

sxa force-pushed the CONTRIBUTING.lts branch from f2d641e to 363fa4b Compare January 18, 2017 12:03

nodejs-github-bot removed lts-watch-v6.x labels Jan 18, 2017

sxa force-pushed the CONTRIBUTING.lts branch 2 times, most recently from 9344866 to 57f7489 Compare January 18, 2017 12:12

doc: CONTRIBUTING.md updates (will squash)

f9bc1e9

sxa force-pushed the CONTRIBUTING.lts branch from 57f7489 to f9bc1e9 Compare January 19, 2017 13:32

gibfahn approved these changes Jan 19, 2017

View reviewed changes

gibfahn added lts-watch-v4.x labels Jan 19, 2017

jasnell requested changes Jan 19, 2017

View reviewed changes

sam-github approved these changes Jan 19, 2017

View reviewed changes

gibfahn mentioned this pull request Feb 1, 2017

doc: add guide for backporting PRs #11099

Closed

2 tasks

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

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

TimothyGu closed this Apr 22, 2017

doc: Update CONTRIBUTING.md with backport info #10343

doc: Update CONTRIBUTING.md with backport info #10343

Conversation

sxa commented Dec 19, 2016

Checklist

Affected core subsystem(s)

Description of change

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 Dec 20, 2016 • edited Loading

jasnell commented Dec 23, 2016

MylesBorins 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

gibfahn Dec 28, 2016 • 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

gibfahn commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn 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

gibfahn commented Jan 19, 2017

gibfahn left a comment

Choose a reason for hiding this comment

gibfahn commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Mar 24, 2017

MylesBorins commented Mar 27, 2017

TimothyGu commented Apr 22, 2017

gibfahn commented Dec 20, 2016 •

edited

Loading

gibfahn Dec 28, 2016 •

edited

Loading