Skip to content

Commit

Permalink
doc: drop minimum waiting time as hard guideline
Browse files Browse the repository at this point in the history
As the [2020 Node.js Contributors Survey][] has shown, the waiting
time for pull requests is a non-trivial obstacle to meaningfully
contributing to Node.js.

To reduce that friction, this PR:

- Drops the 48-hour/7-day waiting times as strict rules.
- Drops the `fast-track` label, which is now the implied default.
- Adds a `wait-for-feedback` label that collaborators can add if they
  think that a pull request *should* remain open for at least 48 hours.
- Reduces the strict requirement for merging something to 1 approval,
  and keep 2 approvals as a strong suggestion.
- Allows immediate reverting of pull requests that have been
  fast-tracked, if deemed necessary.

[2020 Node.js Contributors Survey]: nodejs/TSC#882

Refs: nodejs/TSC#882
Fixes: nodejs#33627
  • Loading branch information
addaleax authored and Trott committed Aug 18, 2020
1 parent 68ccd16 commit 57ab533
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 29 deletions.
44 changes: 21 additions & 23 deletions doc/guides/collaborator-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
* [Code reviews](#code-reviews)
* [Consensus seeking](#consensus-seeking)
* [Waiting for approvals](#waiting-for-approvals)
* [Reverting PRs shortly after landing](#reverting-prs-shortly-after-landing)
* [Testing and CI](#testing-and-ci)
* [Useful CI jobs](#useful-ci-jobs)
* [Starting a CI job](#starting-a-ci-job)
Expand Down Expand Up @@ -95,9 +96,8 @@ request must pass code review and CI before landing into the codebase.

### Code reviews

At least two Collaborators must approve a pull request before the pull request
lands. One Collaborator approval is enough if the pull request has been open
for more than seven days.
At least one Collaborator must approve a pull request before the pull request
lands. Waiting for a second approval is recommended but not mandatory.

Approving a pull request indicates that the Collaborator accepts responsibility
for the change.
Expand Down Expand Up @@ -150,30 +150,28 @@ adding the `tsc-agenda` label to the issue.

### Waiting for approvals

Before landing pull requests, allow 48 hours for input from other Collaborators.
Certain types of pull requests can be fast-tracked and can land after a shorter
delay. For example:
Before landing pull requests, allow for input from other Collaborators.
If you expect that a pull request might raise objections,
wait longer before landing. You can also add the `wait-for-feedback` label
to pull requests; in that case, it is expected that the pull request will not
be merged until at least 48 hours after it has been opened, in order to allow
for everybody to comment on it.

* Focused changes that affect only documentation and/or the test suite:
* `code-and-learn` tasks often fall into this category.
* `good-first-issue` pull requests might also be suitable.
* Changes that fix regressions:
* Regressions that break the workflow (red CI or broken compilation).
* Regressions that happen right before a release, or reported soon after.
For pull requests that are lacking an approval from a member of the relevant
team (e.g. @nodejs/http in the case of HTTP changes), it is recommended to keep
them open for a longer period of time, e.g. several days.

To propose fast-tracking a pull request, apply the `fast-track` label. Then add
a comment that Collaborators can upvote.
### Reverting PRs shortly after landing

If someone disagrees with the fast-tracking request, remove the label. Do not
fast-track the pull request in that case.
If it has been less than 48 hours after a pull request has been opened and it
has already been landed, and you object to it in a way that cannot be addressed
through a new pull request which addresses your concerns, you can open a
pull request containing a revert and merge it without waiting for approvals or
CI runs.

The pull request can be fast-tracked if two Collaborators approve the
fast-tracking request. To land, the pull request itself still needs two
Collaborator approvals and a passing CI.

Collaborators can request fast-tracking of pull requests they did not author.
In that case only, the request itself is also one fast-track approval. Upvote
the comment anyway to avoid any doubt.
If it has been more than 48 hours since the original pull request was opened,
you can still open a revert pull request, but should follow the standard rules
for merging them, i.e. wait for approvals, no objections, and a passing CI run.

### Testing and CI

Expand Down
2 changes: 2 additions & 0 deletions doc/guides/onboarding-extras.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ More than one subsystem may be valid for any particular issue or pull request.
* `discuss`: Things that need larger discussion
* `feature request`: Any issue that requests a new feature
* `good first issue`: Issues suitable for newcomers to fix
* `wait-for-feedback` - Keep this pull request open at least 48 hours before
landing, to give others a chance to weigh in.
* `meta`: Governance, policies, procedures, etc.
* `tsc-agenda`: Open issues and pull requests with this label will be added to
the Technical Steering Committee meeting agenda
Expand Down
16 changes: 10 additions & 6 deletions onboarding.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,15 @@ onboarding session.
reviewers. If you are leaving comments about issues that could be identified
by tools but are not, consider implementing the necessary tooling.
* Minimum wait for comments time
* There is a minimum waiting time which we try to respect for non-trivial
changes so that people who may have important input in such a distributed
project are able to respond.
* For non-trivial changes, leave the pull request open for at least 48 hours.
* Node.js is a distributed project, and it is important that other people
have a chance to weigh in on changes that affect them. We do not enforce
a fixed waiting time, and pull requests that can generally be expected
not to be contentious can be merged as soon as they have approvals and a
passing CI run.
* If in doubt, leave the pull request open for at least 48 hours.
* If you object to a pull request that has already been merged, open a revert
PR. If it has been less then 48 hours since the original PR has been opened,
you can merge the revert PR immediately.
* If a pull request is abandoned, check if they'd mind if you took it over
(especially if it just has nits left).
* Approving a change
Expand Down Expand Up @@ -205,8 +210,7 @@ needs to be pointed out separately during the onboarding.
-1`
* Collaborators are in alphabetical order by GitHub username.
* Optionally, include your personal pronouns.
* Label your pull request with the `doc`, `notable-change`, and `fast-track`
labels.
* Label your pull request with the `doc`, `notable-change` labels.
* Run CI on the PR. Use the `node-test-pull-request` CI task.
* After two Collaborator approvals for the change and two Collaborator approvals
for fast-tracking, land the PR.
Expand Down

0 comments on commit 57ab533

Please sign in to comment.