From 57ab5331d658c22397a62c230f3f4d2ddadba01a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 14 Jun 2020 22:34:18 +0200 Subject: [PATCH] doc: drop minimum waiting time as hard guideline 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]: https://github.com/nodejs/TSC/pull/882 Refs: https://github.com/nodejs/TSC/pull/882 Fixes: https://github.com/nodejs/node/issues/33627 --- doc/guides/collaborator-guide.md | 44 +++++++++++++++----------------- doc/guides/onboarding-extras.md | 2 ++ onboarding.md | 16 +++++++----- 3 files changed, 33 insertions(+), 29 deletions(-) diff --git a/doc/guides/collaborator-guide.md b/doc/guides/collaborator-guide.md index 2867a83df177ce..7f88735a068dbb 100644 --- a/doc/guides/collaborator-guide.md +++ b/doc/guides/collaborator-guide.md @@ -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) @@ -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. @@ -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 diff --git a/doc/guides/onboarding-extras.md b/doc/guides/onboarding-extras.md index 030074093eef36..8ca4a83b34c5e0 100644 --- a/doc/guides/onboarding-extras.md +++ b/doc/guides/onboarding-extras.md @@ -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 diff --git a/onboarding.md b/onboarding.md index 8ec268570e8a66..b8f31f9ce4eb92 100644 --- a/onboarding.md +++ b/onboarding.md @@ -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 @@ -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.