Skip to content
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

Jump to bottom

doc: update collaborator guide #19116

Closed
wants to merge 6 commits into from
Closed

doc: update collaborator guide #19116

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0988b69
doc: update collaborator guide
BridgeAR Mar 3, 2018
b77b56a
fixup: address comments
BridgeAR Mar 4, 2018
0652f24
fixup
BridgeAR Mar 4, 2018
12ef5df
fixup address comments
BridgeAR Mar 6, 2018
a9e01da
fixup
BridgeAR Mar 9, 2018
79c48f5
fixup: address comment
BridgeAR Mar 11, 2018
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
192 changes: 121 additions & 71 deletions COLLABORATOR_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
# Node.js Collaborator Guide

**Contents**
## Contents

* [Issues and Pull Requests](#issues-and-pull-requests)
- [Managing Issues and Pull Requests](#managing-issues-and-pull-requests)
- [Welcoming First-Time Contributors](#welcoming-first-time-contributors)
- [Closing Issues and Pull Requests](#closing-issues-and-pull-requests)
- [Ready to land pull requests](#ready-to-land-pull-requests)
- [Handling own pull requests](#handling-own-pull-requests)
* [Accepting Modifications](#accepting-modifications)
- [Code Reviews and Consensus Seeking](#code-reviews-and-consensus-seeking)
- [Code Reviews](#code-reviews)
- [Consensus Seeking](#consensus-seeking)
- [Waiting for Approvals](#waiting-for-approvals)
- [Testing and CI](#testing-and-ci)
- [Useful CI Jobs](#useful-ci-jobs)
Expand Down Expand Up @@ -45,19 +48,19 @@ understand the project governance model as outlined in

### Managing 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][]. They
may also notify other qualified parties for more input on an issue
or a pull request.
Collaborators should take full responsibility for managing issues and pull
requests they feel qualified to handle. Make sure this is done while being
mindful of these guidelines, the opinions of other Collaborators, and guidance
of the [TSC][]. They may also notify other qualified parties for more input on
an issue or a pull request.
[See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues)

### Welcoming First-Time Contributors

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 ![First-time contributor](./doc/first_timer_badge.png) badge.
identified by the GitHub ![First-time contributor](./doc/first_timer_badge.png)
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
Expand All @@ -75,47 +78,86 @@ Collaborators or additional evidence that the issue has relevance, the
issue may be closed. Remember that issues can always be re-opened if
necessary.

### Ready to land pull requests

A pull request that is still awaiting the minimum review time is considered
Copy link
Member

Choose a reason for hiding this comment

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

Nit: is considered -> may be labeled

Copy link
Member Author

Choose a reason for hiding this comment

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

In this sentence my intention was to explain what ready stands for and not that we should label the PR accordingly. That is done in the sentence afterwards. Do you still want me to change that?

Copy link
Member

Choose a reason for hiding this comment

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

It's a nit; if you think it's better the way it is, then that works for me.

`ready` as soon as the CI has been started, it has at least one approval, and it
has no outstanding review comments. Please always make sure to add the
appropriate `ready` label to the PR in that case and remove it again as soon as
that condition is not met anymore.
Copy link
Member

Choose a reason for hiding this comment

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

I thought ready was ok to land, so CI has run and someone has checked if this could land. So ready should mean "ready to land", not "check CI because this could be ready to land".

Also, it should include a CITGM run for semver-major.

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina When I introduced that label, I didn’t intend it to have that meaning.

Practically speaking, it’s easier this way; you can kick off CI and add the label, because you’re planning to go through the list of open PRs with that label, and can then check CI.

In particular, it avoids having to keep track of which PRs have CIs started and which not.

Copy link
Member Author

Choose a reason for hiding this comment

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

As @addaleax explained it is meant to be the way it is described right now. This has helped me (and probably others just as much, but I can not speak for others) quite a lot already to keep a better overview of PRs that should be checked again.
If everything was already checked there is little point besides the waiting time to have the label at all because in that case the PR should just be merged instead.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that it is easier and it's what we want, and it's probably the right moment to apply a tag. However, I don't think a PR is "ready to land" if it does not have CI passing, so the "ready" term is very misleading to me: I've landed something that should not have landed because of it. It's definitely not ready to land if CI has not been checked for which failures are infrastructure or not.

Copy link
Member

Choose a reason for hiding this comment

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

I'd have to agree with @mcollina on this. Perhaps ready in this case should really mean ready to consider landing. Before the PR can actually land, a look at the CI results is required.


### Handling own pull requests

If you as a Collaborator open a pull request, please always start a CI right
after (see [testing and CI](#testing-and-ci) for further information on how to
do that) and post the link to it as well. Please also make sure that you start a
new CI after each update (due to e.g., a change request in a review or due to
rebasing).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure starting a CI right after opening is the best way to go. I often wait until there are a few reviews to see if I'm going to have to make any updates.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to keep it this way until we get that part automated. Do you feel strongly about it?

See the former discussion for the reasons, e.g., #19116 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR Can we hold off landing this before we have consensus here?

Or, alternatively, could you reword this as a suggestion?

(That is a general feeling I have about this PR, btw: I don’t think we should impose our own workflows on other people.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I personally am not a huge fan about starting a CI always either ^^. The only reason why I think it is good, is that it improves the handling of the PRs as far as I see it.
I also believe that we all want to have a automation of this for collaborators, so I think this is just a intermediate step. I am going to open a issue in build to get some insight in how we might be able to automate it.

This is, by all means, not meant to impose a workflow on someone. Even with the current wording I do not see it as something mandatory, just as something that is really nice to have. I am going to tone down a wording a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworded.

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax @mhdawson is the new wording OK for you? If not: do you have a suggestion how to reword it?


As soon as the PR is ready to land, please go ahead and do so on your own.
Landing your own pull requests distributes the work load for each Collaborator
equally. If it is still awaiting the
[minimum time to land](#waiting-for-approvals), please add the `ready` label to
it so it is obvious that the PR can land as soon as the time ends.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence assumes that a "ready to land" means it has a passing CI. However, in the previous paragraph it's defined as "a PR with CI started", which definitely does not meet our criteria to landing them (we want CI passing, right?).


## 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.
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 must also be tested with CI, before being
landed into the codebase. There may be exception to the latter.
Copy link
Contributor

Choose a reason for hiding this comment

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

"There may be exception to the latter". When? Clarity would be useful here.


### Code Reviews and Consensus Seeking
### Code Reviews

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)
In some cases, it may be necessary to summon 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 you are the first Collaborator to approve a pull request that has no CI yet,
please start one (see [testing and CI](#testing-and-ci) for further information
on how to do that) and post the link to the CI in the PR. Please also start a
new CI in case the PR creator pushed new code since the last CI run (due to
e.g., an addressed review comment or a rebase).

In case there are already enough approvals (`LGTM`), a CI run, and the PR is
open longer than the minimum waiting time without any open comments, please do
not (only) add another approval. Instead go ahead and land the PR after checking
the CI outcome.

### Consensus Seeking

If there is no disagreement amongst Collaborators, a pull request should be
landed given appropriate review, a green CI, and the minimum
[waiting time](#waiting-for-approvals) for a PR. If it is still awaiting the
[minimum time to land](#waiting-for-approvals), please add the `ready` label to
it so it is obvious that the PR can land as soon as the time ends.

Where there is discussion amongst Collaborators, consensus should be sought if
possible. The lack of consensus may indicate the need to elevate discussion to
the TSC for resolution.

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.
Where there is discussion amongst Collaborators, consensus should be
sought if possible. The lack of consensus may indicate the need to
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.
Note that 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 should be landed. Where
there is disagreement among TSC members or objections from one or more
Collaborators, `semver-major` pull requests may be put on the TSC meeting
agenda.

#### Helpful resources

Expand Down Expand Up @@ -147,10 +189,10 @@ CI testing is done.
All bugfixes require a test case which demonstrates the defect. The
test should *fail* before the change, and *pass* after the change.

All pull requests that modify executable code should be subjected to
continuous integration tests on the
[project CI server](https://ci.nodejs.org/).
The pull request should have a CI status indicator if possible.
All pull requests that modify executable code should also include a test case
and be subjected to continuous integration tests on the
[project CI server](https://ci.nodejs.org/). The pull request should have a CI
status indicator if possible.

#### Useful CI Jobs

Expand Down Expand Up @@ -262,7 +304,7 @@ backward-incompatible way) without a deprecation.
Exceptions to this rule may be made in the following cases:

* Adding or removing errors thrown or reported by a Public API;
* Changing error messages;
* Changing error messages for errors without error code;
* Altering the timing and non-internal side effects of the Public API.

Such changes *must* be handled as semver-major changes but MAY be landed
Expand Down Expand Up @@ -432,28 +474,33 @@ The TSC should serve as the final arbiter where required.

## Landing Pull Requests

* Please never use GitHub's green ["Merge Pull Request"](https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github) button.
* If you do, please force-push removing the merge.
* Reasons for not using the web interface button:
* The merge method will add an unnecessary merge commit.
* The squash & merge method has been known to add metadata to the
commit title (the PR #).
* If more than one author has contributed to the PR, keep the most recent
author when squashing.

Review the commit message to ensure that it adheres to the guidelines outlined
in the [contributing](./doc/guides/contributing/pull-requests.md#commit-message-guidelines) guide.

Add all necessary [metadata](#metadata) to commit messages before landing.

See the commit log for examples such as
[this one](https://github.com/nodejs/node/commit/b636ba8186) if unsure
exactly how to format your commit messages.
1. Never use GitHub's green ["Merge Pull Request"][] button. Reasons for not
using the web interface button:
* The merge method will add an unnecessary merge commit.
* The squash & merge method has been known to add metadata to the commit
title (the PR #).
* If more than one author has contributed to the PR, keep the most recent
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this point a bit confusing? If this is an advice, should we get it out of "Reasons for not using..." part? If this is a green button artifact, should the "keep" become "keeps" and should the culprit method be mentioned?

Copy link
Member

Choose a reason for hiding this comment

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

In fact, I'd strictly be in favor of keeping code by authors who are not core collaborators in PRs even if they are not the latest. I think that if there are multiple contributors we should keep commits from all of them (perhaps with the same commit message)

Copy link
Member

Choose a reason for hiding this comment

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

It requires judgment, particularly around how significant the contribution is. If someone contributed something significant, squashing commits in a way that removes them as an author should certainly be avoided. That can look like (or can actually be) stealing credit for someone else's work. On the other hand, we do have to make sure that all tests pass with each commit, and sometimes squashing is the most reasonable way to achieve that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the point that this is confusing but I would really like to keep this out of the scope of this PR. I did not change this part, so I guess that is fine?

Copy link
Member

@Trott Trott Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, yes, hadn't noticed that this content has not changed and this is just a formatting/white-space change to this part. Yeah, the comments should be addressed in another PR IMO.

author when squashing.
1. Make sure the CI is done and the result is green. If the CI is not green,
check for flaky tests and infrastructure failures. Please check if those were
already reported in the appropriate repository ([node][flaky tests] and
[build](https://github.com/nodejs/build/issues)) or not and open new issues
in case they are not. If no CI was run or the run is outdated because code
was pushed after the last run, please first start a new CI and wait for the
result. If no CI is required, please leave a comment in case none is already
present.
1. Review the commit message to ensure that it adheres to the guidelines
outlined in the [contributing][] guide.
1. Add all necessary [metadata](#metadata) to commit messages before landing.
See the commit log for examples such as [this
one](https://github.com/nodejs/node/commit/b636ba8186) if unsure exactly how
to format your commit messages.

Additionally:
- Double check PRs to make sure the person's _full name_ and email

* Double check PRs to make sure the person's _full name_ and email
address are correct before merging.
- All commits should be self-contained (meaning every commit should pass all
* All commits should be self-contained (meaning every commit should pass all
tests). This makes it much easier when bisecting to find a breaking change.

### Using `git-node`
Expand Down Expand Up @@ -541,7 +588,7 @@ This will open a screen like this (in the default shell editor):
```text
pick 6928fc1 crypto: add feature A
pick 8120c4c add test for feature A
pick 51759dc feature B
pick 51759dc crypto: feature B
pick 7d6f433 test for feature B

# Rebase f9456a2..7d6f433 onto f9456a2
Expand Down Expand Up @@ -569,7 +616,7 @@ previous commit:
```text
pick 6928fc1 crypto: add feature A
fixup 8120c4c add test for feature A
pick 51759dc feature B
pick 51759dc crypto: feature B
fixup 7d6f433 test for feature B
```

Expand All @@ -578,7 +625,7 @@ Replace `pick` with `reword` to change the commit message:
```text
reword 6928fc1 crypto: add feature A
fixup 8120c4c add test for feature A
reword 51759dc feature B
reword 51759dc crypto: feature B
fixup 7d6f433 test for feature B
```

Expand Down Expand Up @@ -655,12 +702,12 @@ hint: See the 'Note about fast-forwards' in 'git push --help' for details.
```

That means a commit has landed since your last rebase against `upstream/master`.
To fix this, fetch, rebase, run the tests again (to make sure no interactions
between your changes and the new changes cause any problems), and push again:
To fix this, pull with rebase from upstream and run the tests again (to make
sure no interactions between your changes and the new changes cause any
problems), and push again:

```sh
git fetch upstream
git rebase upstream/master
git pull upstream master --rebase
make -j4 test
git push upstream master
```
Expand Down Expand Up @@ -714,15 +761,15 @@ and impact of the changes on the code, the risk to ecosystem stability
incurred by accepting the change, and the expected benefit that landing the
commit will have for the ecosystem.

Any collaborator who feels a semver-minor commit should be landed in an LTS
Any Collaborator who feels a semver-minor commit should be landed in an LTS
branch should attach the `lts-agenda` label to the pull request. The LTS WG
will discuss the issue and, if necessary, will escalate the issue up to the
TSC for further discussion.

#### How are LTS Branches Managed?

There are currently two LTS branches: `v6.x` and `v4.x`. Each of these is paired
with a staging branch: `v6.x-staging` and `v4.x-staging`.
There are multiple LTS branches, e.g. `v8.x` and `v6.x`. Each of these is paired
with a staging branch: `v8.x-staging` and `v6.x-staging`.

As commits land on the master branch, they are cherry-picked back to each
staging branch as appropriate. If the commit applies only to the LTS branch, the
Expand All @@ -731,16 +778,16 @@ pulled from the staging branch into the LTS branch only when a release is
being prepared and may be pulled into the LTS branch in a different order
than they were landed in staging.

Any collaborator may land commits into a staging branch, but only the release
Any Collaborator may land commits into a staging branch, but only the release
team should land commits into the LTS branch while preparing a new
LTS release.

#### How can I help?

When you send your pull request, consider including information about
whether your change is breaking. If you think your patch can be backported,
please feel free to include that information in the PR thread. For more
information on backporting, please see the [backporting guide][].
When you send your pull request, please include information about whether your
change is breaking. If you think your patch can be backported, please include
that information in the PR thread or your PR description. For more information
on backporting, please see the [backporting guide][].

Several LTS related issue and PR labels have been provided:

Expand All @@ -753,7 +800,7 @@ Several LTS related issue and PR labels have been provided:
* `land-on-v4.x` - tells the release team that the commit should be landed
in a future v4.x release

Any collaborator can attach these labels to any PR/issue. As commits are
Any Collaborator can attach these labels to any PR/issue. As commits are
landed into the staging branches, the `lts-watch-` label will be removed.
Likewise, as commits are landed in a LTS release, the `land-on-` label will
be removed.
Expand All @@ -769,6 +816,7 @@ release. This process of making a release will be a collaboration between the
LTS working group and the Release team.

[backporting guide]: doc/guides/backporting-to-release-lines.md
[contributing]: ./doc/guides/contributing/pull-requests.md#commit-message-guidelines
[Stability Index]: doc/api/documentation.md#stability-index
[Enhancement Proposal]: https://github.com/nodejs/node-eps
[`--pending-deprecation`]: doc/api/cli.md#--pending-deprecation
Expand All @@ -779,3 +827,5 @@ LTS working group and the Release team.
[TSC]: https://github.com/nodejs/TSC
[node-core-utils-issues]: https://github.com/nodejs/node-core-utils/issues
[node-core-utils-credentials]: https://github.com/nodejs/node-core-utils#setting-up-credentials
["Merge Pull Request"]: https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github
[flaky tests]: https://github.com/nodejs/node/issues?q=is%3Aopen+is%3Aissue+label%3A%22CI+%2F+flaky+test%22
23 changes: 15 additions & 8 deletions doc/onboarding.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ onboarding session.
`semver-major` label
* When adding a `semver-*` label, add a comment explaining why you're adding
it. Do it right away so you don't forget!
* Please add the `ready` label for PRs where:
* the CI has been started,
Copy link
Member

Choose a reason for hiding this comment

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

The CI has been successfully run.

Copy link
Contributor

Choose a reason for hiding this comment

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

If CI was required, as some of this documentation indicates it may not be.

* that have no outstanding review comments and
* at least one approval,
* pending the CI outcome,
* the 48/72 hour waiting rule and
* the necessary LGs for semver-major.

* [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues)
* This will come more naturally over time
Expand All @@ -107,11 +114,11 @@ onboarding session.

* The primary goal is for the codebase to improve.
* Secondary (but not far off) is for the person submitting code to succeed. A
pull request from a new contributor is an opportunity to grow the community.
pull request from a new contributor is an opportunity to grow the community.
* Review a bit at a time. Do not overwhelm new contributors.
* It is tempting to micro-optimize. Don't succumb to that temptation. We
change V8 often. Techniques that provide improved performance today may be
unnecessary in the future.
change V8 often. Techniques that provide improved performance today may be
unnecessary in the future.
* Be aware: Your opinion carries a lot of weight!
* Nits (requests for small changes that are not essential) are fine, but try to
avoid stalling the pull request.
Expand All @@ -122,15 +129,15 @@ onboarding session.
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.
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
(72 hours on a weekend).
(72 hours on a weekend).
* If a pull request is abandoned, check if they'd mind if you took it over
(especially if it just has nits left).
(especially if it just has nits left).
* Approving a change
* Collaborators indicate that they have reviewed and approve of the changes in
a pull request using Github’s approval interface
a pull request using Github’s approval interface
* Some people like to comment `LGTM` (“Looks Good To Me”)
* You have the authority to approve any other collaborator’s work.
* You cannot approve your own pull requests.
Expand Down