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.

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

doc: clarify the review and landing process #10202

Closed
wants to merge 10 commits into from
83 changes: 75 additions & 8 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -243,18 +243,85 @@ If in doubt, you can always ask for guidance in the Pull Request or on
[IRC in the #node-dev channel](https://webchat.freenode.net?channels=node-dev&uio=d4).

Feel free to post a comment in the Pull Request to ping reviewers if you are
awaiting an answer on something.
awaiting an answer on something. If you encounter words or acronyms that
seem unfamiliar, check out this
[glossary](https://sites.google.com/a/chromium.org/dev/glossary).
Copy link
Member

Choose a reason for hiding this comment

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

Oh… thank you! I didn’t know this existed.


Note that multiple commits often get squashed when they are landed (see the
notes about [commit squashing](#commit-squashing)).

### Step 8: Landing

Once your Pull Request has been reviewed and approved by at least one Node.js
Collaborators (often by saying LGTM, or Looks Good To Me), and as long as
there is consensus (no objections from a Collaborator), a
Collaborator can merge the Pull Request . GitHub often shows the Pull Request as
`Closed` at this point, but don't worry. If you look at the branch you raised
your Pull Request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!
In order to get landed, a Pull Request needs to be reviewed and
[approved](#getting-approvals-for-your-pull-request) by
at least one Node.js Collaborator and pass a
[CI (Continuous Integration) test run](#ci-testing).
After that, as long as there are no objections
from a Collaborator, the Pull Request can be merged. If you find your
Pull Request waiting longer than you expect, see the
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).

When a collaborator lands your Pull Request, they will post
a comment to the Pull Request page mentioning the commit(s) it
landed as. GitHub often shows the Pull Request as `Closed` at this
point, but don't worry. If you look at the branch you raised your
Pull Request against (probably `master`), you should see a commit with
your name on it. Congratulations and thanks for your contribution!

## Additional Notes

### Commit Squashing

When the commits in your Pull Request get landed, they will be squashed
into one commit per logical change, with metadata added to the commit
message (including links to the Pull Request, links to relevant issues,
and the names of the reviewers). The commit history of your Pull Request,
however, will stay intact on the Pull Request page.

For the size of "one logical change",
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
can be a good example. It touches the implementation, the documentation,
and the tests, but is still one logical change. In general, the tests should
always pass when each individual commit lands on the master branch.

### Getting Approvals for Your Pull Request

A Pull Request is approved either by saying LGTM, which stands for
"Looks Good To Me", or by using GitHub's Approve button.
GitHub's Pull Request review feature can be used during the process.
For more information, check out
[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g)
or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/).

After you push new changes to your branch, you need to get
approval for these new changes again, even if GitHub shows "Approved"
because the reviewers have hit the buttons before.

### CI Testing

Every Pull Request needs to be tested
to make sure the it works on the platforms that Node.js
Copy link
Contributor

@thefourtheye thefourtheye Dec 20, 2016

Choose a reason for hiding this comment

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

make sure that it

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks for spotting this one :)

supports. This is done by running the code through the CI system.

Only a Collaborator can request a CI run. Usually one of them will do it
for you as approvals for the Pull Request come in.
If not, you can ask a Collaborator to request a CI run.

### Waiting Until the Pull Request Gets Landed

A Pull Request needs to stay open for at least 48 hours (72 hours on a
weekend) from when it is submitted, even after it gets approved and
passes the CI. This is to make sure that everyone has a chance to
Copy link
Member

Choose a reason for hiding this comment

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

Microscopic nit: There is a trailing space at the end of this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should have trimmed the spaces :P. Addressed. Thanks!

weigh in. If the changes are trivial, collaborators may decide it
doesn't need to wait. A Pull Request may well take longer to be
merged in. All these precautions are important because Node.js is
widely used, so don't be discouraged!

### Check Out the Collaborator's Guide

If you want to know more about the code review and the landing process,
you can take a look at the
[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md).

<a id="developers-certificate-of-origin"></a>
## Developer's Certificate of Origin 1.1