-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Closed
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
cc22d35
doc: clarify the review and landing process
joyeecheung e8153b7
doc: clarify the landing process
joyeecheung 0bb76ba
doc: explain the waiting time of semver-major PRs
joyeecheung 32ab27a
doc: put the notes and details in the back
joyeecheung 9bda4a4
doc: improve the clarification
joyeecheung dbd83ef
doc: more fixes about the process
joyeecheung 96e0189
doc: amends the contributing guide
joyeecheung 246f52a
doc: testing docs, PR history preservation
joyeecheung e9c53f3
doc: fix typo
joyeecheung 41e4e93
doc: trim trailling spaces
joyeecheung File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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). | ||
|
||
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 that it works on the platforms that Node.js | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.