doc: improve collaborator guide, doc fast-tracking process #17056
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,31 @@ | |
|
|
||
| * [Issues and Pull Requests](#issues-and-pull-requests) | ||
| * [Accepting Modifications](#accepting-modifications) | ||
| - [First Time Contributions](#first-time-contributions) | ||
| - [Code Reviews and Consensus Seeking](#code-reviews-and-consensus-seeking) | ||
| - [Waiting for Approvals](#waiting-for-approvals) | ||
| - [Testing and CI](#testing-and-ci) | ||
| - [Useful CI Jobs](#useful-ci-jobs) | ||
| - [Internal vs. Public API](#internal-vs-public-api) | ||
| - [Breaking Changes](#breaking-changes) | ||
| - [Breaking Changes and Deprecations](#breaking-changes-and-deprecations) | ||
| - [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) | ||
| - [When Breaking Changes Actually Break Things](#when-breaking-changes-actually-break-things) | ||
| - [Reverting commits](#reverting-commits) | ||
| - [Introducing New Modules](#introducing-new-modules) | ||
| - [Deprecations](#deprecations) | ||
| - [Involving the TSC](#involving-the-tsc) | ||
| * [Landing Pull Requests](#landing-pull-requests) | ||
| - [Technical HOWTO](#technical-howto) | ||
| - [Troubleshooting](#troubleshooting) | ||
| - [I Just Made a Mistake](#i-just-made-a-mistake) | ||
| - [Long Term Support](#long-term-support) | ||
| - [What is LTS?](#what-is-lts) | ||
| - [How does LTS work?](#how-does-lts-work) | ||
| - [Landing semver-minor commits in LTS](#landing-semver-minor-commits-in-lts) | ||
| - [How are LTS Branches Managed?](#how-are-lts-branches-managed) | ||
| - [How can I help?](#how-can-i-help) | ||
| - [How is an LTS release cut?](#how-is-an-lts-release-cut) | ||
|
|
||
| This document contains information for Collaborators of the Node.js | ||
| project regarding maintaining the code, documentation and issues. | ||
|
|
@@ -24,14 +40,10 @@ understand the project governance model as outlined in | |
|
|
||
| ## Issues and Pull Requests | ||
|
|
||
| Collaborators should feel free to take full responsibility for | ||
| managing issues and pull requests they feel qualified to handle, as | ||
| long as this is done while being mindful of these guidelines, the | ||
| opinions of other Collaborators and guidance of the [TSC][]. | ||
|
|
||
| Collaborators may **close** any issue or pull request they believe is | ||
| not relevant for the future of the Node.js project. Where this is | ||
|
|
@@ -41,36 +53,46 @@ Collaborators or additional evidence that the issue has relevance, the | |
| issue may be closed. Remember that issues can always be re-opened if | ||
| necessary. | ||
|
|
||
| [See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
|
|
||
| ## Accepting Modifications | ||
|
|
||
| All modifications to the Node.js code and documentation should be | ||
| performed via GitHub pull requests, including modifications by | ||
| Collaborators and TSC members. A pull request must be reviewed, and usually | ||
| must also be tested with CI, before being landed into the codebase. | ||
|
|
||
| ### First Time Contributions | ||
|
|
||
| Courtesy should **always** be shown to individuals submitting issues and pull | ||
| requests to the Node.js project. Be welcoming to first time contributors, | ||
| identified by the GitHub  badge. | ||
|
|
||
| For first time contributors, check if the commit author is the same as the | ||
| pull request author, and ask if they have configured their git | ||
| username and email to their liking as per [this guide][git-username]. | ||
| This is to make sure they would be promoted to "contributor" once | ||
| their pull request gets landed. | ||
|
|
||
| ### Code Reviews and Consensus Seeking | ||
|
|
||
| All pull requests must be reviewed and accepted by a Collaborator with | ||
| sufficient expertise who is able to take full responsibility for the | ||
| change. In the case of pull requests proposed by an existing | ||
| Collaborator, an additional Collaborator is required for sign-off. | ||
|
|
||
| In some cases, it may be necessary to summon a qualified Collaborator | ||
| or a Github team to a pull request for review by @-mention. | ||
| [See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
|
|
||
| If you are unsure about the modification and are not prepared to take | ||
| full responsibility for the change, defer to another Collaborator. | ||
|
|
||
| If any Collaborator objects to a change *without giving any additional | ||
| explanation or context*, and the objecting Collaborator fails to respond to | ||
| explicit requests for explanation or context within a reasonable period of | ||
| time, the objection may be dismissed. Note that this does not apply to | ||
| objections that are explained. | ||
|
|
||
| For non-breaking changes, if there is no disagreement amongst | ||
| Collaborators, a pull request may be landed given appropriate review. | ||
|
|
@@ -80,12 +102,33 @@ elevate discussion to the TSC for resolution (see below). | |
|
|
||
| Breaking changes (that is, pull requests that require an increase in | ||
| the major version number, known as `semver-major` changes) must be | ||
| [elevated for review by the TSC](#involving-the-tsc). | ||
| This does not necessarily mean that the PR must be put onto the TSC meeting | ||
| agenda. If multiple TSC members approve (`LGTM`) the PR and no Collaborators | ||
| oppose the PR, it can be landed. Where there is disagreement among TSC members | ||
| or objections from one or more Collaborators, `semver-major` pull requests | ||
| should be put on the TSC meeting agenda. | ||
|
|
||
| ### Waiting for Approvals | ||
|
|
||
| Before landing pull requests, sufficient time should be left for input | ||
| from other Collaborators. In general, leave at least 48 hours during the | ||
| week and 72 hours over weekends to account for international time | ||
| differences and work schedules. However, certain types of pull requests | ||
| 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. | ||
|
There was a problem hiding this comment. I'm 👎 on considering improving performance as a fast-track issue. In my experience, it's the contrary. There was a problem hiding this comment. Also, personally I agree with @mcollina , I don't recall seeing a performance optimization PR being fast-tracked before? There was a problem hiding this comment. This wording was added in c52e43d, a long, long time before my PR. I’m okay with removing performance optimizations this as well. There was a problem hiding this comment. 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.
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. There was a problem hiding this comment. @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. |
||
| * Focused changes that affect only documentation and/or the test suite. | ||
| `code-and-learn` and `good-first-issue` pull requests typically fall | ||
| into this category. | ||
| * 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 | ||
|
There was a problem hiding this comment. The number 3 here needs more discussion There was a problem hiding this comment. I'm 👍 on fast-tracking everything that fixed a regression There was a problem hiding this comment. I would go with 2, and be more explicit: |
||
| approve both the pull request and the fast-tracking request. | ||
|
|
||
| ### Testing and CI | ||
|
|
||
| All bugfixes require a test case which demonstrates the defect. The | ||
| test should *fail* before the change, and *pass* after the change. | ||
|
|
@@ -94,12 +137,6 @@ All pull requests that modify executable code should be subjected to | |
| continuous integration tests on the | ||
| [project CI server](https://ci.nodejs.org/). | ||
|
|
||
| #### Useful CI Jobs | ||
|
|
||
| * [`node-test-pull-request`](https://ci.nodejs.org/job/node-test-pull-request/) | ||
|
|
@@ -172,20 +209,8 @@ using an API in a manner currently undocumented achieves a particular useful | |
| result, a decision will need to be made whether or not that falls within the | ||
| supported scope of that API; and if it does, it should be documented. | ||
|
|
||
| See [Breaking Changes to Internal Elements](#breaking-changes-to-internal-elements) | ||
| on how to handle that type of changes. | ||
|
There was a problem hiding this comment. Nit: |
||
|
|
||
| ### Breaking Changes | ||
|
|
||
|
|
@@ -200,6 +225,20 @@ changing error messages in any way, altering expected timing of an event (e.g. | |
| moving from sync to async responses or vice versa), and changing the | ||
| non-internal side effects of using a particular API. | ||
|
|
||
| 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. | ||
|
|
||
|
|
||
| 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 | ||
|
There was a problem hiding this comment. They're not subject to the deprecation policy, but they are still tagged As a side note, it seems odd that this section doesn't mention that you have to put the There was a problem hiding this comment. Hmm, yes this seem to belong to the "Breaking Changes and Deprecations" section. Thanks for catching that. |
||
| dependencies when it is known or expected that breaking changes to error | ||
| handling may have been made. Additional CI testing may be required. | ||
|
|
||
| #### Breaking Changes and Deprecations | ||
|
|
||
| With a few notable exceptions outlined below, when backwards incompatible | ||
| changes to a *Public* API are necessary, the existing API *must* be deprecated | ||
| *first* and the new API either introduced in parallel or added after the next | ||
|
|
@@ -219,19 +258,26 @@ without a [Deprecation cycle](#deprecation-cycle). | |
| From time-to-time, in particularly exceptional cases, the TSC may be asked to | ||
| consider and approve additional exceptions to this rule. | ||
|
|
||
| For more information, see [Deprecations](#deprecations). | ||
|
|
||
| #### Breaking Changes to Internal Elements | ||
|
|
||
| Breaking changes to internal elements are permitted in semver-patch or | ||
| semver-minor commits but Collaborators should take significant care when | ||
| making and reviewing such changes. Before landing such commits, an effort | ||
| must be made to determine the potential impact of the change in the ecosystem | ||
| by analyzing current use and by validating such changes through ecosystem | ||
| testing using the [Canary in the Goldmine](https://github.com/nodejs/citgm) | ||
| tool. If a change cannot be made without ecosystem breakage, then TSC review is | ||
| required before landing the change as anything less than semver-major. | ||
|
|
||
| If a determination is made that a particular internal API (for instance, an | ||
| underscore `_` prefixed property) is sufficiently relied upon by the ecosystem | ||
| such that any changes may break user code, then serious consideration should be | ||
| given to providing an alternative Public API for that functionality before any | ||
| breaking changes are made. | ||
|
|
||
| #### When Breaking Changes Actually Break Things | ||
|
|
||
| Because breaking (semver-major) changes are permitted to land on the master | ||
| branch at any time, at least some subset of the user ecosystem may be adversely | ||
|
|
@@ -349,12 +395,13 @@ Changes" section of the release notes. | |
|
|
||
| ### Involving the TSC | ||
|
|
||
| Collaborators may opt to elevate pull requests or issues to the [TSC][] for | ||
| discussion by assigning the `tsc-review` label or @-mentioning the | ||
| `@nodejs/tsc` Github team. This should be done where a pull request: | ||
|
|
||
| - is labeled `semver-major`, or | ||
| - has a significant impact on the codebase, or | ||
| - is inherently controversial, or | ||
| - has failed to reach consensus amongst the Collaborators who are | ||
| actively participating in the discussion. | ||
|
|
||
|
|
@@ -681,3 +728,4 @@ LTS working group and the Release team. | |
| [Enhancement Proposal]: https://github.com/nodejs/node-eps | ||
| [git-username]: https://help.github.com/articles/setting-your-username-in-git/ | ||
| [`node-core-utils`]: https://github.com/nodejs/node-core-utils | ||
| [TSC]: https://github.com/nodejs/TSC | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really liked that it was the first sentence in this guide, just after the TOC. So IMHO it should stay there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...I think we can just move the section up because this does not seem to be about "Accepting Modifications" anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"