doc: improve collaborator guide, doc fast-tracking process #17056

joyeecheung · 2017-11-15T19:14:21Z

doc: reorganize collaborator guide

Add sections about first time contributions, code reviews
and seeking consensus, waiting for approvals, testing and CI
Move paragraphs to more suitable sections
Update table of contents

doc: document the fast-tracking process

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

* Add sections about first time contributions, code reviews and seeking consensus, waiting for approvals, testing and CI * Move paragraphs to more suitable sections * Update table of contents

joyeecheung · 2017-11-15T19:19:15Z

COLLABORATOR_GUIDE.md

+* Changes that revert commit(s) and/or fix regressions.
+
+When a pull request is deemed suitable to be fast-tracked, label it with
+`fast-track`. The pull request can be landed once more than 3 collaborators


The number 3 here needs more discussion

I'm 👍 on fast-tracking everything that fixed a regression

I would go with 2, and be more explicit:
The PR can be landed after 2 or more collaborators (excluding the author) have approved it.

joyeecheung · 2017-11-15T19:20:24Z

cc @nodejs/tsc @nodejs/collaborators

mcollina · 2017-11-15T19:25:37Z

COLLABORATOR_GUIDE.md

+can be fast-tracked and may be landed after a shorter delay:
+
+* Trivial changes, for example, those fixing minor bugs or improving
+  performance without affecting API or causing other wide-reaching impact.


I'm 👎 on considering improving performance as a fast-track issue. In my experience, it's the contrary.

This was added in #16135, should we remove that one? cc @addaleax

Also, personally I agree with @mcollina , I don't recall seeing a performance optimization PR being fast-tracked before?

This wording was added in c52e43d, a long, long time before my PR.

I’m okay with removing performance optimizations this as well.

Maybe a topic for more general discussion, but at least for me the triviality of the change isn't usually a reason to fast-track. My experience is that no matter how trivial the change, there's always the possibility that someone disagrees or has nits.

For me the reason to fast-track is because it is actually beneficial, e.g.

It's a fix we want to land in a release line

It's a code-and-learn PR, and we want to give people a good first experience (and maybe even land on the day)

It unbreaks CI on master

So for example if it fixes a regression but won't go into a release for a couple of weeks, what's the reason to fast-track? Same for small changes.

@gibfahn I agree but these are just some examples, that's why we also need a rule about "n approvals to both the PR and the fast-tracking request". If a PR should not be fast-tracked, people can object to that fast-tracking request without objecting to the PR.

mcollina · 2017-11-15T19:26:06Z

COLLABORATOR_GUIDE.md

+* Changes that revert commit(s) and/or fix regressions.
+
+When a pull request is deemed suitable to be fast-tracked, label it with
+`fast-track`. The pull request can be landed once more than 3 collaborators


I'm 👍 on fast-tracking everything that fixed a regression

refack · 2017-11-15T19:42:49Z

COLLABORATOR_GUIDE.md

+
+### First Time Contributions
+
+Courtesy should **always** be shown to individuals submitting issues and pull


I really liked that it was the first sentence in this guide, just after the TOC. So IMHO it should stay there.

Hmm...I think we can just move the section up because this does not seem to be about "Accepting Modifications" anyway.

On a second thought, I don't think this paragraph should appear before explaining that "a collaborator can manage issues and pull request", it looks a bit surprising to start explaining "how you should do something" before even saying "you can do this now"

gibfahn · 2017-11-15T20:11:53Z

COLLABORATOR_GUIDE.md

+Purely additive changes (e.g. adding new events to EventEmitter
+implementations, adding new arguments to a method in a way that allows
+existing code to continue working without modification, or adding new
+properties to an options argument) are handled as semver-minor changes.


nit: remove handled as

gibfahn · 2017-11-15T20:15:00Z

COLLABORATOR_GUIDE.md

-given to providing an alternative Public API for that functionality before any
-breaking changes are made.
+See [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements)
+on how to handle that type of changes.


Nit: that type -> those types (or changes -> change)

gibfahn · 2017-11-15T21:40:51Z

COLLABORATOR_GUIDE.md

+can be fast-tracked and may be landed after a shorter delay:
+
+* Trivial changes, for example, those fixing minor bugs or improving
+  performance without affecting API or causing other wide-reaching impact.


Maybe a topic for more general discussion, but at least for me the triviality of the change isn't usually a reason to fast-track. My experience is that no matter how trivial the change, there's always the possibility that someone disagrees or has nits.

For me the reason to fast-track is because it is actually beneficial, e.g.

It's a fix we want to land in a release line

It's a code-and-learn PR, and we want to give people a good first experience (and maybe even land on the day)

It unbreaks CI on master

So for example if it fixes a regression but won't go into a release for a couple of weeks, what's the reason to fast-track? Same for small changes.

gibfahn · 2017-11-15T21:44:13Z

COLLABORATOR_GUIDE.md

+Note that errors thrown, along with behaviors and APIs implemented by
+dependencies of Node.js (e.g. those originating from V8) are generally not
+under the control of Node.js and therefore *are not directly subject to this
+policy*. However, care should still be taken when landing updates to


They're not subject to the deprecation policy, but they are still tagged semver-major, still need the TSC signoffs, and still don't get backported to any release lines right? Why the move up to this section?

As a side note, it seems odd that this section doesn't mention that you have to put the semver-major label on the PR (I know it wasn't there before). Maybe it's covered somewhere else.

Hmm, yes this seem to belong to the "Breaking Changes and Deprecations" section. Thanks for catching that.

…change to 2 approvals

joyeecheung · 2017-11-16T09:40:59Z

@mcollina @addaleax @gibfahn @refack I've moved the paragraphs a bit, deleted the bug-fix and optimization examples, and changed the number of required approvals to 2 (I've taken a look into the recent fast-tracked PRs and I think 2 should be safe enough. We can always revert if someone objects later). PTAL.

tniessen · 2017-11-16T23:36:55Z

COLLABORATOR_GUIDE.md

+* Changes that revert commit(s) and/or fix regressions.
+
+When a pull request is deemed suitable to be fast-tracked, label it with
+`fast-track`. The pull request can be landed once more than 2 collaborators


I think this should be 2 or more then. LGTM otherwise.

joyeecheung · 2017-11-17T14:07:50Z

@tniessen Updated to 2 or more collaborators and added a sentence about CI is still required for fast-track PRs (it's pretty obvious, but still should be mentioned )

mhdawson

LGTM

joyeecheung · 2017-11-18T07:39:31Z

Landed as 0a1fba0, thanks!

* Add sections about first time contributions, code reviews and seeking consensus, waiting for approvals, testing and CI * Move paragraphs to more suitable sections * Update table of contents * Document the fast-tracking process PR-URL: #17056 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

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

joyeecheung added 2 commits November 16, 2017 03:17

doc: reorganize collaborator guide

546cea1

* Add sections about first time contributions, code reviews and seeking consensus, waiting for approvals, testing and CI * Move paragraphs to more suitable sections * Update table of contents

doc: document the fast-tracking process

d5b3ce8

joyeecheung force-pushed the fast-track branch from 16a4ffa to d5b3ce8 Compare November 15, 2017 19:18

joyeecheung commented Nov 15, 2017

View reviewed changes

mcollina reviewed Nov 15, 2017

View reviewed changes

refack reviewed Nov 15, 2017

View reviewed changes

gibfahn reviewed Nov 15, 2017

View reviewed changes

joyeecheung added 2 commits November 16, 2017 17:32

[squash] reword and moving paragraphs to better places

75902aa

[squash] remove bug-fix and optimizations as fast-tracking examples, …

00f7b9f

…change to 2 approvals

jasnell approved these changes Nov 16, 2017

View reviewed changes

tniessen approved these changes Nov 16, 2017

View reviewed changes

[squash] reword fast-track requirements

d793f21

mhdawson approved these changes Nov 17, 2017

View reviewed changes

gireeshpunathil approved these changes Nov 18, 2017

View reviewed changes

joyeecheung closed this Nov 18, 2017

joyeecheung added the meta Issues and PRs related to the general management of the project. label Nov 18, 2017

joyeecheung mentioned this pull request Nov 18, 2017

PRChecker: support new label fast-track nodejs/node-core-utils#104

Merged

vsemozhetbyt mentioned this pull request Nov 18, 2017

Todo: Skip the wait check for specific PRs nodejs/node-core-utils#94

Closed

2 tasks

refack mentioned this pull request Nov 26, 2017

doc: clarify fast-track of reversions #17332

Merged

2 tasks

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

MylesBorins added the land-on-v6.x label Dec 19, 2017

gibfahn added the land-on-v8.x label Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

MylesBorins mentioned this pull request Dec 20, 2017

v6.12.3 proposal #17776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: improve collaborator guide, doc fast-tracking process #17056

doc: improve collaborator guide, doc fast-tracking process #17056

joyeecheung commented Nov 15, 2017

joyeecheung Nov 15, 2017

mcollina Nov 15, 2017

refack Nov 15, 2017

joyeecheung commented Nov 15, 2017

mcollina Nov 15, 2017

joyeecheung Nov 15, 2017

joyeecheung Nov 15, 2017

addaleax Nov 15, 2017

gibfahn Nov 15, 2017

joyeecheung Nov 16, 2017

mcollina Nov 15, 2017

refack Nov 15, 2017 •

edited

Loading

joyeecheung Nov 16, 2017

joyeecheung Nov 16, 2017 •

edited

Loading

gibfahn Nov 15, 2017

gibfahn Nov 15, 2017

gibfahn Nov 15, 2017

gibfahn Nov 15, 2017

joyeecheung Nov 16, 2017

joyeecheung commented Nov 16, 2017 •

edited

Loading

tniessen Nov 16, 2017

joyeecheung commented Nov 17, 2017

mhdawson left a comment

joyeecheung commented Nov 18, 2017


		### First Time Contributions

		Courtesy should always be shown to individuals submitting issues and pull

doc: improve collaborator guide, doc fast-tracking process #17056

doc: improve collaborator guide, doc fast-tracking process #17056

Conversation

joyeecheung commented Nov 15, 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

joyeecheung commented Nov 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

joyeecheung commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Nov 17, 2017

mhdawson left a comment

Choose a reason for hiding this comment

joyeecheung commented Nov 18, 2017

refack Nov 15, 2017 •

edited

Loading

joyeecheung Nov 16, 2017 •

edited

Loading

joyeecheung commented Nov 16, 2017 •

edited

Loading