Skip to content

doc: drop minimum waiting time as hard guideline #33879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
addaleax wants to merge 1 commit into from
Closed

doc: drop minimum waiting time as hard guideline #33879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions doc/guides/collaborator-guide.md
View file
Open in desktop
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.
Comment thread
jasnell marked this conversation as resolved.
Outdated

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.
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collaborators that are in different timezones or away on local bank holidays... that's why the 48 hour rule exists.

This "one approval rule" is basically a 1-2 for collaborators from the same timezone.

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.
Copy link
Copy Markdown
Member

@Qard Qard Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could auto-suggest the 48 hour rule on certain critical systems? For example, if a PR changes openssl or low-level parts of http/https/http2, or if it changes something marked stable and targets an LTS branch, it would automatically get the wait-for-feedback label.

Copy link
Copy Markdown
Member

@targos targos Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTS branches are managed by the Release WG. This PR shouldn't affect them.


* 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
Comment thread
jasnell marked this conversation as resolved.
Outdated
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.
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Jun 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect this revert rule will cause no end of bad blood and internet drama...

Copy link
Copy Markdown
Member

@Flarna Flarna Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that revert without wait/approval may result in issues.
Maybe we should limit this to some scenarios or at least provide some examples when this is acceptable?

Copy link
Copy Markdown
Member

@targos targos Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/releasers we'll need to keep an eye on this and adapt release tooling if reverts become more present

Copy link
Copy Markdown
Member

@richardlau richardlau Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Based on what I've seen in the Release WG mentoring sessions where we've been walked through the triage process of whether something is ready to land on LTS release branches it is already difficult to ascertain links between a PR and any subsequent reverts/fixups.

Comment on lines 169 to 170
Copy link
Copy Markdown
Contributor

@mmarchini mmarchini Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add link to "Reverting a commit" so folks know what are the right steps to revert a commit.

Suggested change
pull request containing a revert and merge it without waiting for approvals or
CI runs.
[pull request containing a revert](#reverting-commits) and merge it without
waiting for approvals or CI runs.

Copy link
Copy Markdown
Contributor

@mmarchini mmarchini Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also make sure ncu can recognize revert commits and consider those as valid (otherwise we won't be able to use the commit queue to land).


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
View file
Open in desktop
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
Copy link
Copy Markdown
Member

@Trott Trott Jun 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The labels are in alphabetical/lexical order right now. I'd prefer we keep it that way and move this one to the end of this specific list (after tsc-agenda).

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
View file
Open in desktop
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
Copy link
Copy Markdown
Member

@Trott Trott Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a fixed waiting time, and pull requests that can generally be expected
a fixed waiting time. Pull requests that are expected

not to be contentious can be merged as soon as they have approvals and a
Copy link
Copy Markdown
Member

@Trott Trott Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
not to be contentious can be merged as soon as they have approvals and a
to be uncontentious 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.
Copy link
Copy Markdown
Member

@Trott Trott Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Label your pull request with the `doc`, `notable-change` labels.
* Label your pull request with the `doc` and `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