From b831beaff4db9075594dd78ee2f7a28683aa21c1 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 30 Aug 2016 11:20:55 -0700 Subject: [PATCH 1/6] 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. --- doc/onboarding.md | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index 991809df1ea40a..ec9e9c5783db49 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -116,39 +116,34 @@ 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 usually sufficient, except for semver-major changes. + * The more the better. + * Breaking changes must be LGTM'ed by at least two CTC members. - * be sure to wait before merging non-trivial changes - * 48 hours for non-trivial changes, and 72 hours on weekends. + * Wait before merging non-trivial changes. + * 48 hours during the week and 72 hours on weekends. - * **make sure to run the PR through CI before merging!** (Except for documentation PRs) + * **Run the PR through CI before merging!** + * An exception can be made for documentation-only 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. Post to `#node-dev` (IRC) if you need to 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 +155,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.) From 8a98226017e7b2759d884379bfda22336c59f235 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 30 Aug 2016 19:06:20 -0700 Subject: [PATCH 2/6] squash: address nits --- doc/onboarding.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index ec9e9c5783db49..02f50df608fbc0 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -125,10 +125,13 @@ onboarding session. * An exception is made for release commits only. - * One "LGTM" is usually sufficient, except for semver-major changes. - * The more the better. + * 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 may not land. Your options in such a situation (aside from abandoning the change) are: + * Engage those with objections to persuade them to drop their objections. + * Alter your changes so that the objections are dropped. + * Escalate to the CTC with the `ctc-agenda` label. Do not do this without exhausting the other options first. * Wait before merging non-trivial changes. * 48 hours during the week and 72 hours on weekends. @@ -140,7 +143,9 @@ onboarding session. * What if something goes wrong? * Ping a CTC member. * `#node-dev` on freenode - * Force-pushing to fix things after is allowed for ~10 minutes. Avoid it if you can. Post to `#node-dev` (IRC) if you need to force push. + * 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: Details @@ -167,7 +172,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 ``". From 2f5bfe367764c302b240ac5d756f624a8be307a2 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 31 Aug 2016 13:00:40 -0700 Subject: [PATCH 3/6] squash: rewording --- doc/onboarding.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index 02f50df608fbc0..c3c6ef9f2f9746 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -128,10 +128,12 @@ onboarding session. * 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 may not land. Your options in such a situation (aside from abandoning the change) are: - * Engage those with objections to persuade them to drop their objections. - * Alter your changes so that the objections are dropped. - * Escalate to the CTC with the `ctc-agenda` label. Do not do this without exhausting the other options first. + * 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. From 95354ba6beba84d45dc2048a98ebb3a9dbc2709c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 31 Aug 2016 14:12:34 -0700 Subject: [PATCH 4/6] squash: add trivial example --- doc/onboarding.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index c3c6ef9f2f9746..d3d45193769b26 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -137,7 +137,7 @@ onboarding session. * 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. * **Run the PR through CI before merging!** * An exception can be made for documentation-only PRs. From ee1eb7e4c83f86f8dd4c71d08560fc5ae0fb4b43 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 31 Aug 2016 14:13:47 -0700 Subject: [PATCH 5/6] squash: more trivial info --- doc/onboarding.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index d3d45193769b26..6c222c8d49138e 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -137,7 +137,7 @@ onboarding session. * 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. + * 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. * **Run the PR through CI before merging!** * An exception can be made for documentation-only PRs. From 2bd288cf38f0f76414044d7ce03769eb6f1b1db0 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Wed, 31 Aug 2016 14:15:06 -0700 Subject: [PATCH 6/6] squash: addons --- doc/onboarding.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index 6c222c8d49138e..9434e9b5fc0a91 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -140,7 +140,7 @@ onboarding session. * 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. * **Run the PR through CI before merging!** - * An exception can be made for documentation-only PRs. + * 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!) * What if something goes wrong? * Ping a CTC member.