Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: update landing pr info in onboarding doc #8344

Closed
wants to merge 6 commits into from
Closed

doc: update landing pr info in onboarding doc #8344

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b831bea
doc: update landing pr info in onboarding doc
Trott Aug 30, 2016
8a98226
squash: address nits
Trott Aug 31, 2016
2f5bfe3
squash: rewording
Trott Aug 31, 2016
95354ba
squash: add trivial example
Trott Aug 31, 2016
ee1eb7e
squash: more trivial info
Trott Aug 31, 2016
2bd288c
squash: addons
Trott Aug 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 26 additions & 23 deletions doc/onboarding.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,39 +116,41 @@ 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or corrections to the commit log within the 10 minute window?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, I suppose yes. No one has ever been confused about that, as far as I know, though. And I'd prefer to avoid introducing content that my increase cognitive load without providing valuable information. If you want to propose some language that you think would work, I'm open to it.



* 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.
Copy link
Contributor

@Fishrock123 Fishrock123 Aug 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still important

edit: nvm, see it below...

* 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.
Expand All @@ -160,6 +162,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.)
Expand All @@ -171,7 +174,7 @@ Landing a PR
* `Reviewed-By: human <email>`
* 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: <full-pr-url>`
* `git push upstream master`
* close the original PR with "Landed in `<commit hash>`".
Expand Down