Skip to content

Commit 01a2299

Browse files
joyeecheungMylesBorins
authored andcommitted
doc: clarify the review and landing process
Adds/mentions: - Link to glossary - Commit squashing and CI run - 48/72 hour wait and PR review feature - Extra notes section - "Landed in <sha>" comment PR-URL: #10202 Ref: #10151 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
1 parent c889338 commit 01a2299

File tree

1 file changed

+75
-8
lines changed

1 file changed

+75
-8
lines changed

CONTRIBUTING.md

Lines changed: 75 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -248,18 +248,85 @@ If in doubt, you can always ask for guidance in the Pull Request or on
248248
[IRC in the #node-dev channel](https://webchat.freenode.net?channels=node-dev&uio=d4).
249249

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

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

254258
### Step 8: Landing
255259

256-
Once your Pull Request has been reviewed and approved by at least one Node.js
257-
Collaborators (often by saying LGTM, or Looks Good To Me), and as long as
258-
there is consensus (no objections from a Collaborator), a
259-
Collaborator can merge the Pull Request . GitHub often shows the Pull Request as
260-
`Closed` at this point, but don't worry. If you look at the branch you raised
261-
your Pull Request against (probably `master`), you should see a commit with
262-
your name on it. Congratulations and thanks for your contribution!
260+
In order to get landed, a Pull Request needs to be reviewed and
261+
[approved](#getting-approvals-for-your-pull-request) by
262+
at least one Node.js Collaborator and pass a
263+
[CI (Continuous Integration) test run](#ci-testing).
264+
After that, as long as there are no objections
265+
from a Collaborator, the Pull Request can be merged. If you find your
266+
Pull Request waiting longer than you expect, see the
267+
[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).
268+
269+
When a collaborator lands your Pull Request, they will post
270+
a comment to the Pull Request page mentioning the commit(s) it
271+
landed as. GitHub often shows the Pull Request as `Closed` at this
272+
point, but don't worry. If you look at the branch you raised your
273+
Pull Request against (probably `master`), you should see a commit with
274+
your name on it. Congratulations and thanks for your contribution!
275+
276+
## Additional Notes
277+
278+
### Commit Squashing
279+
280+
When the commits in your Pull Request get landed, they will be squashed
281+
into one commit per logical change, with metadata added to the commit
282+
message (including links to the Pull Request, links to relevant issues,
283+
and the names of the reviewers). The commit history of your Pull Request,
284+
however, will stay intact on the Pull Request page.
285+
286+
For the size of "one logical change",
287+
[0b5191f](https://github.com/nodejs/node/commit/0b5191f15d0f311c804d542b67e2e922d98834f8)
288+
can be a good example. It touches the implementation, the documentation,
289+
and the tests, but is still one logical change. In general, the tests should
290+
always pass when each individual commit lands on the master branch.
291+
292+
### Getting Approvals for Your Pull Request
293+
294+
A Pull Request is approved either by saying LGTM, which stands for
295+
"Looks Good To Me", or by using GitHub's Approve button.
296+
GitHub's Pull Request review feature can be used during the process.
297+
For more information, check out
298+
[the video tutorial](https://www.youtube.com/watch?v=HW0RPaJqm4g)
299+
or [the official documentation](https://help.github.com/articles/reviewing-changes-in-pull-requests/).
300+
301+
After you push new changes to your branch, you need to get
302+
approval for these new changes again, even if GitHub shows "Approved"
303+
because the reviewers have hit the buttons before.
304+
305+
### CI Testing
306+
307+
Every Pull Request needs to be tested
308+
to make sure that it works on the platforms that Node.js
309+
supports. This is done by running the code through the CI system.
310+
311+
Only a Collaborator can request a CI run. Usually one of them will do it
312+
for you as approvals for the Pull Request come in.
313+
If not, you can ask a Collaborator to request a CI run.
314+
315+
### Waiting Until the Pull Request Gets Landed
316+
317+
A Pull Request needs to stay open for at least 48 hours (72 hours on a
318+
weekend) from when it is submitted, even after it gets approved and
319+
passes the CI. This is to make sure that everyone has a chance to
320+
weigh in. If the changes are trivial, collaborators may decide it
321+
doesn't need to wait. A Pull Request may well take longer to be
322+
merged in. All these precautions are important because Node.js is
323+
widely used, so don't be discouraged!
324+
325+
### Check Out the Collaborator's Guide
326+
327+
If you want to know more about the code review and the landing process,
328+
you can take a look at the
329+
[collaborator's guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md).
263330

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

0 commit comments

Comments
 (0)