From 76a3050c349d03b3a52611128d65856213c8be52 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 30 Aug 2016 11:20:55 -0700 Subject: [PATCH] doc: update landing pr info in onboarding doc Reword and re-organize. Only significant content change is to specifically call out the two-CTC-member-LGTM requirement for semver-major changes. PR-URL: https://github.com/nodejs/node/pull/8344 Reviewed-By: James M Snell Reviewed-By: Evan Lucas --- doc/onboarding.md | 48 ++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index 991809df1e..918757f218 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -116,39 +116,40 @@ onboarding session. * The remaining elements on the form are typically unchanged with the exception of `POST_STATUS_TO_PR`. Check that if you want a CI status indicator to be automatically inserted into the PR. -## process for getting code in +## Landing PRs: Overview - * the collaborator guide is a great resource: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto + * The [Collaborator Guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto) is a great resource. - * no one (including TSC or CTC members) pushes directly to master without review - * an exception is made for release commits only + * No one (including TSC or CTC members) pushes directly to master without review. + * An exception is made for release commits only. - * one "LGTM" is usually sufficient, except for semver-major changes - * the more the better - * semver-major (breaking) changes must be reviewed in some form by the CTC + * One `LGTM` is sufficient, except for semver-major changes. + * More than one is better. + * Breaking changes must be LGTM'ed by at least two CTC members. + * If one or more Collaborators object to a change, it should not land until + the objection is addressed. The options for such a situation include: + * Engaging those with objections to determine a viable path forward; + * Altering the pull request to address the objections; + * Escalating the discussion to the CTC using the `ctc-agenda` label. This should only be done after other options have been exhausted. + * Wait before merging non-trivial changes. + * 48 hours during the week and 72 hours on weekends. + * An example of a trivial change would be correcting the misspelling of a single word in a documentation file. This sort of change still needs to receive at least one `LGTM` but it does not need to wait 48 hours before landing. - * be sure to wait before merging non-trivial changes - * 48 hours for non-trivial changes, and 72 hours on weekends. + * **Run the PR through CI before merging!** + * An exception can be made for documentation-only PRs as long as it does not include the `addons.md` documentation file. (Example code from that document is extracted and built as part of the tests!) - - * **make sure to run the PR through CI before merging!** (Except for documentation PRs) - - - * once code is ready to go in: - * [**See "Landing PRs"**](#landing-prs) below - - - * what if something goes wrong? - * ping a CTC member + * What if something goes wrong? + * Ping a CTC member. * `#node-dev` on freenode - * force-pushing to fix things after is allowed for ~10 minutes, be sure to notify people in IRC if you need to do this, but avoid it - * Info on PRs that don't like to apply found under [**"If `git am` fails"**](./onboarding-extras.md#if-git-am-fails). + * Force-pushing to fix things after is allowed for ~10 minutes. Avoid it if you can. + * Use `--force-with-lease` to minimize the chance of overwriting someone else's change. + * Post to `#node-dev` (IRC) if you force push. -## Landing PRs +## Landing PRs: Details * Please never use GitHub's green "Merge Pull Request" button. * If you do, please force-push removing the merge. @@ -160,6 +161,7 @@ Update your `master` branch (or whichever branch you are landing on, almost alwa Landing a PR * if it all looks good, `curl -L 'url-of-pr.patch' | git am` + * If `git am` fails, see [the relevant section of the Onboarding Extras doc](./onboarding-extras.md#if-git-am-fails). * `git rebase -i upstream/master` * squash into logical commits if necessary * `./configure && make -j8 test` (`-j8` builds node in parallel with 8 threads. adjust to the number of cores (or processor-level threads) your processor has (or slightly more) for best results.) @@ -171,7 +173,7 @@ Landing a PR * `Reviewed-By: human ` * Easiest to use `git log` then do a search * (`/Name` + `enter` (+ `n` as much as you need to) in vim) - * Only include collaborators who have commented "LGTM" + * Only include collaborators who have commented `LGTM` * `PR-URL: ` * `git push upstream master` * close the original PR with "Landed in ``".