Skip to content

Commit 5c417ee

Browse files
TrottMylesBorins
authored andcommitted
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: #8344 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
1 parent 763fa85 commit 5c417ee

File tree

1 file changed

+25
-23
lines changed

1 file changed

+25
-23
lines changed

doc/onboarding.md

+25-23
Original file line numberDiff line numberDiff line change
@@ -116,39 +116,40 @@ onboarding session.
116116
* 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.
117117

118118

119-
## process for getting code in
119+
## Landing PRs: Overview
120120

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.
122122

123123

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.
126126

127127

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.
131136

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.
132140

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!)
135143

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.
146146
* `#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.
149150

150151

151-
## Landing PRs
152+
## Landing PRs: Details
152153

153154
* Please never use GitHub's green "Merge Pull Request" button.
154155
* 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
160161
Landing a PR
161162

162163
* 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).
163165
* `git rebase -i upstream/master`
164166
* squash into logical commits if necessary
165167
* `./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
171173
* `Reviewed-By: human <email>`
172174
* Easiest to use `git log` then do a search
173175
* (`/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`
175177
* `PR-URL: <full-pr-url>`
176178
* `git push upstream master`
177179
* close the original PR with "Landed in `<commit hash>`".

0 commit comments

Comments
 (0)