| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
1 parent 763fa85 commit 5c417ee
1 file changed
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -116,39 +116,40 @@ onboarding session. | |||
| 116 | 116 | * 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. | |
| 117 | 117 | ||
| 118 | 118 | ||
| 119 | - ## process for getting code in | ||
| 119 | + ## Landing PRs: Overview | ||
| 120 | 120 | ||
| 121 | - * the collaborator guide is a great resource: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto | ||
| 121 | + * The [Collaborator Guide](https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto) is a great resource. | ||
| 122 | 122 | ||
| 123 | 123 | ||
| 124 | - * no one (including TSC or CTC members) pushes directly to master without review | ||
| 125 | - * an exception is made for release commits only | ||
| 124 | + * No one (including TSC or CTC members) pushes directly to master without review. | ||
| 125 | + * An exception is made for release commits only. | ||
| 126 | 126 | ||
| 127 | 127 | ||
| 128 | - * one "LGTM" is usually sufficient, except for semver-major changes | ||
| 129 | - * the more the better | ||
| 130 | - * semver-major (breaking) changes must be reviewed in some form by the CTC | ||
| 128 | + * One `LGTM` is sufficient, except for semver-major changes. | ||
| 129 | + * More than one is better. | ||
| 130 | + * Breaking changes must be LGTM'ed by at least two CTC members. | ||
| 131 | + * If one or more Collaborators object to a change, it should not land until | ||
| 132 | + the objection is addressed. The options for such a situation include: | ||
| 133 | + * Engaging those with objections to determine a viable path forward; | ||
| 134 | + * Altering the pull request to address the objections; | ||
| 135 | + * Escalating the discussion to the CTC using the `ctc-agenda` label. This should only be done after other options have been exhausted. | ||
| 131 | 136 | ||
| 137 | + * Wait before merging non-trivial changes. | ||
| 138 | + * 48 hours during the week and 72 hours on weekends. | ||
| 139 | + * 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. | ||
| 132 | 140 | ||
| 133 | - * be sure to wait before merging non-trivial changes | ||
| 134 | - * 48 hours for non-trivial changes, and 72 hours on weekends. | ||
| 141 | + * **Run the PR through CI before merging!** | ||
| 142 | + * 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!) | ||
| 135 | 143 | ||
| 136 | - | ||
| 137 | - * **make sure to run the PR through CI before merging!** (Except for documentation PRs) | ||
| 138 | - | ||
| 139 | - | ||
| 140 | - * once code is ready to go in: | ||
| 141 | - * [**See "Landing PRs"**](#landing-prs) below | ||
| 142 | - | ||
| 143 | - | ||
| 144 | - * what if something goes wrong? | ||
| 145 | - * ping a CTC member | ||
| 144 | + * What if something goes wrong? | ||
| 145 | + * Ping a CTC member. | ||
| 146 | 146 | * `#node-dev` on freenode | |
| 147 | - * 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 | ||
| 148 | - * Info on PRs that don't like to apply found under [**"If `git am` fails"**](./onboarding-extras.md#if-git-am-fails). | ||
| 147 | + * Force-pushing to fix things after is allowed for ~10 minutes. Avoid it if you can. | ||
| 148 | + * Use `--force-with-lease` to minimize the chance of overwriting someone else's change. | ||
| 149 | + * Post to `#node-dev` (IRC) if you force push. | ||
| 149 | 150 | ||
| 150 | 151 | ||
| 151 | - ## Landing PRs | ||
| 152 | + ## Landing PRs: Details | ||
| 152 | 153 | ||
| 153 | 154 | * Please never use GitHub's green "Merge Pull Request" button. | |
| 154 | 155 | * 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 | |||
| 160 | 161 | Landing a PR | |
| 161 | 162 | ||
| 162 | 163 | * if it all looks good, `curl -L 'url-of-pr.patch' | git am` | |
| 164 | + * If `git am` fails, see [the relevant section of the Onboarding Extras doc](./onboarding-extras.md#if-git-am-fails). | ||
| 163 | 165 | * `git rebase -i upstream/master` | |
| 164 | 166 | * squash into logical commits if necessary | |
| 165 | 167 | * `./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 | |||
| 171 | 173 | * `Reviewed-By: human <email>` | |
| 172 | 174 | * Easiest to use `git log` then do a search | |
| 173 | 175 | * (`/Name` + `enter` (+ `n` as much as you need to) in vim) | |
| 174 | - * Only include collaborators who have commented "LGTM" | ||
| 176 | + * Only include collaborators who have commented `LGTM` | ||
| 175 | 177 | * `PR-URL: <full-pr-url>` | |
| 176 | 178 | * `git push upstream master` | |
| 177 | 179 | * close the original PR with "Landed in `<commit hash>`". | |
| Back | FazBrowse Home | New Git URL |
0 commit comments