From f7c0eb8ba6af6b9d70a274a3d075432097ffa68b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 10 Dec 2016 03:11:19 +0800 Subject: [PATCH] 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 " comment PR-URL: https://github.com/nodejs/node/pull/10202 Ref: https://github.com/nodejs/node/pull/10151 Reviewed-By: Anna Henningsen Reviewed-By: Evan Lucas Reviewed-By: Gibson Fahnestock --- CONTRIBUTING.md | 83 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 75 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eb00977e92..6d419aa495 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -248,18 +248,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 +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). ## Developer's Certificate of Origin 1.1