doc: update backporting guide #13749

refack · 2017-06-17T20:32:06Z

also update STYLE_GUIDE comment about emdashes

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Trott · 2017-06-17T20:34:51Z

doc/STYLE_GUIDE.md

@@ -68,3 +68,5 @@

 [plugin]: http://editorconfig.org/#download
 [Oxford comma]: https://en.wikipedia.org/wiki/Serial_comma
+[emdashes]: https://en.wikipedia.org/wiki/Dash


Nit: Perhaps this should link to https://en.wikipedia.org/wiki/Dash#Em_dash instead?

vsemozhetbyt · 2017-06-17T21:39:19Z

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

+what can be landed. Our LTS release lines (`v4.x` and `v6.x` as of March 2017)
+require that commits mature in the Current release for at least 2 weeks before
+they can be landed in an LTS staging branch. Please see the [LTS Plan][] for
+more information. Only after "maturation" will those the commits be


those the commits -> those commits?

gibfahn

Assorted nits (many from the existing text, but while you're here...)

gibfahn · 2017-06-17T22:24:16Z

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

+| `v8.x`       | `v8.x-staging` | Current     |
+| `v7.x`       | `v7.x-staging` | Maintenance |
+| `v6.x`       | `v6.x-staging` | LTS         |
+| `v4.x`       | `v4.x-staging` | Maintenance |


I didn't realise this was in the guide, I remember @MylesBorins suggested we remove it in the original PR, as it will get out of date every few months.

As this info is basically duplicated in the LTS schedule, maybe link to that in ## Staging branches and remove the whole ### Active staging branches section.

gibfahn · 2017-06-17T22:24:36Z

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
-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.
+line (e.g. `backport-requested-v8.x`), specifying to our tooling that this


v8.x -> vN.x

gibfahn · 2017-06-17T22:26:00Z

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

-for more information. After that time, if the commit can be cherry-picked
-cleanly from master, then nothing needs to be done. If not, a backport pull
-request will need to be submitted.
+what can be landed. Our LTS release lines (`v4.x` and `v6.x` as of March 2017)


How about:

(v4.xandv6.x as of March 2017) -> (see the [LTS Plan][]) and then remove Please see the [LTS Plan][] for more information. from the next line.

gibfahn · 2017-06-17T22:30:30Z

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

-The releaser will then add a comment that a backport is needed.
+line (e.g. `backport-requested-v8.x`), specifying to our tooling that this
+pull request should not be included. The releaser will then add a comment
+that a backport is needed.


that a backport is needed. -> requesting that a backport pull request be made

gibfahn · 2017-06-17T22:35:45Z

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

+      format — `(v7.x backport) #<PR_NUMBER> - <commit title>`.
+      Example: `(v7.x backport) #10157 - process: improve performance of nextTick`
+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a referance to the original PR


referance -> reference

NAck. It's an MD feature:

gibfahn · 2017-06-17T22:36:24Z

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

+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a referance to the original PR
+10. Replace the `backport-requested-v7.x` label on the original PR
+    with `backported-to-v7.x`.


I don't think this should be done until the PR lands.

Moved it out of the numbered list, PTAL.

gibfahn · 2017-06-17T22:38:20Z

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

+    with `backported-to-v7.x`.
+
+*Note*: If the backport pull request is different than the original, it should
+be reviewed the same way a new pull request is reviewed.


All backport PRs are reviewed, what exactly is your meaning here?

I have no Idea, just reformatted from previous text.
Maybe this alludes to the case when the cherry-pick had conflicts that had to be manually resolved.
Or cases like #13750 where I had to add unrelated code i.e. d429bc6

gibfahn · 2017-06-17T22:40:52Z

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

+7. Make sure `make -j4 test` passes.
+8. Push the changes to your fork
+9. Open a pull request:
+   1. Be sure to target the `v7.x-staging` branch in the pull request.


This is maybe just me, but I feel like a. b. c. is more natural than another set of numbers.

This still needs doing.

NAck. It's an MD feature:

(pasted this before in the wrong comment)

refack · 2017-06-21T00:20:19Z

@gibfahn PTAL

gibfahn

Mostly done, one nit and one thing I just noticed we don't specify.

gibfahn · 2017-06-21T07:40:54Z

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

+7. Make sure `make -j4 test` passes.
+8. Push the changes to your fork
+9. Open a pull request:
+   1. Be sure to target the `v7.x-staging` branch in the pull request.


This still needs doing.

gibfahn · 2017-06-21T07:42:24Z

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

+      format — `(v7.x backport) #<PR_NUMBER> - <commit title>`.
+      Example: `(v7.x backport) #10157 - process: improve performance of nextTick`
+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a reference to the original PR


While you're here, could you add a note saying that you should run CI as well? Just node-test-pull-request with the default settings.

Refs: #13835 (comment)

refack

CR for PR tile

refack · 2017-06-22T19:06:24Z

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

+9. Open a pull request:
+   1. Be sure to target the `v7.x-staging` branch in the pull request.
+   2. Include the backport target in the pull request title in the following
+      format — `[v7.x backport] #<PR_NUMBER> - <commit title>`.


I'm asking to change the format of the titles from parans '(' to brackets '[' for personal reasons:

Green arrow is an issue with new comments
Red arrow is a backport PR

[] seems reasonable, but why require the #PR NUMBER? That seems like noise, especially if you have the commit message in the PR description (the PR-URL is a clickable link so more useful). Also doesn't work for multiple PRs backported together.

Ack. No #PR NUMBER

gibfahn

More nits

gibfahn · 2017-06-22T21:09:32Z

doc/STYLE_GUIDE.md

@@ -40,8 +40,8 @@
 * When documenting APIs, note the version the API was introduced in at
  the end of the section. If an API has been deprecated, also note the first
  version that the API appeared deprecated in.
-* When using dashes, use emdashes ("—", Option+Shift+"-" on macOS) surrounded by
-  spaces, per the New York Times usage.
+* When using dashes, use [emdashes][] ("—" or `Option+Shift+"-"` on macOS)


emdashes -> Em dashes

gibfahn · 2017-06-22T21:11:13Z

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

-* After creating the branch, apply the changes to the branch. The cherry-pick
-  will likely fail due to conflicts. In that case, you will see something this:
+4. After creating the branch, apply the changes to the branch. The cherry-pick
+   will likely fail due to conflicts. In that case, you will see something this:


this -> like this

gibfahn · 2017-06-22T21:12:00Z

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

+9. Open a pull request:
+   1. Be sure to target the `v7.x-staging` branch in the pull request.
+   2. Include the backport target in the pull request title in the following
+      format — `[v7.x backport] #<PR_NUMBER> - <commit title>`.


[] seems reasonable, but why require the #PR NUMBER? That seems like noise, especially if you have the commit message in the PR description (the PR-URL is a clickable link so more useful). Also doesn't work for multiple PRs backported together.

gibfahn · 2017-06-22T21:14:11Z

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

+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a reference to the original PR
+   5. Run a [`node-test-pull-request`][] CI job (with `REBASE_UNTO` set to the
+      default `<pr base barnch>`)


barnch -> branch

gibfahn · 2017-06-22T21:14:25Z

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

+      Example: `[v7.x backport] #10157 - process: improve performance of nextTick`
+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a reference to the original PR
+   5. Run a [`node-test-pull-request`][] CI job (with `REBASE_UNTO` set to the


UNTO -> ONTO

vsemozhetbyt · 2017-06-23T11:20:57Z

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

@@ -74,6 +74,8 @@ hint: and commit the result with 'git commit'
   4. In the description add a reference to the original PR
   5. Run a [`node-test-pull-request`][] CI job (with `REBASE_UNTO` set to the
      default `<pr base barnch>`)
+10. If during the review process conflicts arise, use the following to rebase:  
+    `git pull --rebase upstream v6.x-staging`


Maybe we better use v6.x in all the examples instead of v7.x? The latter is in the maintenance phase, there will be hardly any backports for it.

😄 just had the same idea!

refack

Addressed

refack · 2017-06-23T11:42:27Z

doc/STYLE_GUIDE.md

@@ -40,8 +40,8 @@
 * When documenting APIs, note the version the API was introduced in at
  the end of the section. If an API has been deprecated, also note the first
  version that the API appeared deprecated in.
-* When using dashes, use emdashes ("—", Option+Shift+"-" on macOS) surrounded by
-  spaces, per the New York Times usage.
+* When using dashes, use [emdashes][] ("—" or `Option+Shift+"-"` on macOS)


refack · 2017-06-23T11:44:06Z

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

-* After creating the branch, apply the changes to the branch. The cherry-pick
-  will likely fail due to conflicts. In that case, you will see something this:
+4. After creating the branch, apply the changes to the branch. The cherry-pick
+   will likely fail due to conflicts. In that case, you will see something this:


refack · 2017-06-23T11:44:51Z

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

+9. Open a pull request:
+   1. Be sure to target the `v7.x-staging` branch in the pull request.
+   2. Include the backport target in the pull request title in the following
+      format — `[v7.x backport] #<PR_NUMBER> - <commit title>`.


Ack. No #PR NUMBER

refack · 2017-06-23T11:45:11Z

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

+      Example: `[v7.x backport] #10157 - process: improve performance of nextTick`
+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a reference to the original PR
+   5. Run a [`node-test-pull-request`][] CI job (with `REBASE_UNTO` set to the


refack · 2017-06-23T11:45:20Z

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

+   3. Check the checkbox labelled "Allow edits from maintainers".
+   4. In the description add a reference to the original PR
+   5. Run a [`node-test-pull-request`][] CI job (with `REBASE_UNTO` set to the
+      default `<pr base barnch>`)


refack · 2017-06-23T11:48:36Z

@gibfahn @vsemozhetbyt PTAL

vsemozhetbyt

But I think this also needs to be reviewed by some collaborators that bear LTS releases.

refack · 2017-06-23T12:41:53Z

@addaleax @MylesBorins You've been requesting backports, PTAL?...

gibfahn

LGTM, thanks for doing this @refack

* also update STYLE_GUIDE comment about Em dashes PR-URL: nodejs#13749 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

* also update STYLE_GUIDE comment about Em dashes PR-URL: #13749 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

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

Trott reviewed Jun 17, 2017

View reviewed changes

refack force-pushed the backport-guide-learing-by-editing branch from 372f293 to 0312e28 Compare June 17, 2017 20:38

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

vsemozhetbyt reviewed Jun 17, 2017

View reviewed changes

refack mentioned this pull request Jun 17, 2017

doc: adds options for cpp linting to help section of Windows build script #11992

Closed

3 tasks

gibfahn suggested changes Jun 17, 2017

View reviewed changes

gibfahn reviewed Jun 17, 2017

View reviewed changes

refack mentioned this pull request Jun 21, 2017

Question about backporting #13833

Closed

gibfahn reviewed Jun 21, 2017

View reviewed changes

refack commented Jun 22, 2017

View reviewed changes

refack self-assigned this Jun 22, 2017

gibfahn reviewed Jun 22, 2017

View reviewed changes

vsemozhetbyt reviewed Jun 23, 2017

View reviewed changes

refack commented Jun 23, 2017

View reviewed changes

refack force-pushed the backport-guide-learing-by-editing branch from fcb21b9 to e673666 Compare June 23, 2017 11:47

vsemozhetbyt approved these changes Jun 23, 2017

View reviewed changes

gibfahn approved these changes Jun 23, 2017

View reviewed changes

doc: update backporting guide

e3ea0fc

* also update STYLE_GUIDE comment about Em dashes PR-URL: nodejs#13749 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

refack force-pushed the backport-guide-learing-by-editing branch from e673666 to e3ea0fc Compare June 23, 2017 19:24

refack merged commit e3ea0fc into nodejs:master Jun 23, 2017

addaleax mentioned this pull request Jun 24, 2017

v8.1.3 proposal #13861

Merged

addaleax pushed a commit that referenced this pull request Jun 24, 2017

doc: update backporting guide

a63e54a

* also update STYLE_GUIDE comment about Em dashes PR-URL: #13749 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

addaleax mentioned this pull request Jun 24, 2017

v8.2.0 proposal #13744

Merged

refack deleted the backport-guide-learing-by-editing branch July 2, 2017 02:34

MylesBorins pushed a commit that referenced this pull request Jul 17, 2017

doc: update backporting guide

092bba5

* also update STYLE_GUIDE comment about Em dashes PR-URL: #13749 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

refack removed their assignment Oct 20, 2018

doc: update backporting guide #13749

doc: update backporting guide #13749

Conversation

refack commented Jun 17, 2017

Checklist

Affected core subsystem(s)

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

Choose a reason for hiding this comment

refack commented Jun 21, 2017

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

refack 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

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

refack 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

refack commented Jun 23, 2017

vsemozhetbyt left a comment

Choose a reason for hiding this comment

refack commented Jun 23, 2017

gibfahn left a comment

Choose a reason for hiding this comment