mirror of https://github.com/lukechilds/node.git
Browse Source
PR-URL: https://github.com/nodejs/node/pull/3726 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>process-exit-stdio-flushing
Jeremiah Senkpiel
9 years ago
2 changed files with 304 additions and 0 deletions
@ -0,0 +1,105 @@ |
|||||
|
## Who to CC in issues |
||||
|
|
||||
|
* `lib/buffer`: @trevnorris |
||||
|
* `lib/child_process`: @cjihrig, @bnoordhuis, @piscisaereus |
||||
|
* `lib/cluster`: @cjihrig, @bnoordhuis, @piscisaereus |
||||
|
* `lib/{crypto,tls,https}`: @indutny, @shigeki, @nodejs/crypto |
||||
|
* `lib/domains`: @misterdjules |
||||
|
* `lib/{_}http{*}`: @indutny, @bnoordhuis, @nodejs/http |
||||
|
* `lib/net`: @indutny, @bnoordhuis, @piscisaereus, @chrisdickinson, @nodejs/streams |
||||
|
* `lib/{_}stream{s|*}`: @nodejs/streams |
||||
|
* `lib/repl`: @fishrock123 |
||||
|
* `lib/timers`: @fishrock123, @misterdjules |
||||
|
* `lib/zlib`: @indutny, @bnoordhuis |
||||
|
|
||||
|
* `src/async-wrap.*`: @trevnorris |
||||
|
* `src/node_crypto.*`: @indutny, @shigeki, @nodejs/crypto |
||||
|
|
||||
|
* `test/*`: @nodejs/testing, @trott |
||||
|
|
||||
|
* `tools/eslint`, `.eslintrc`: @silverwind, @trott |
||||
|
|
||||
|
* upgrading v8: @bnoordhuis / @targos / @ofrobots |
||||
|
* upgrading npm: @thealphanerd, @fishrock123 |
||||
|
|
||||
|
|
||||
|
When things need extra attention, are controversial, or `semver-major`: @nodejs/ctc |
||||
|
|
||||
|
If you cannot find who to cc for a file, `git shortlog -n -s <file>` may help. |
||||
|
|
||||
|
|
||||
|
## Labels |
||||
|
|
||||
|
### By Subsystem |
||||
|
|
||||
|
We generally sort issues by a concept of "subsystem" so that we know what part(s) of the codebase it touches. |
||||
|
|
||||
|
**Subsystems generally are**: |
||||
|
|
||||
|
* `lib/*.js` |
||||
|
* `doc`, `build`, `tools`, `test`, `deps`, `lib / src` (special), and there may be others. |
||||
|
* `meta` for anything non-code (process) related |
||||
|
|
||||
|
There may be more than one subsystem valid for any particular issue / PR. |
||||
|
|
||||
|
|
||||
|
### General |
||||
|
|
||||
|
Please use these when possible / appropriate |
||||
|
|
||||
|
* `confirmed-bug` - Bugs you have verified exist |
||||
|
* `discuss` - Things that need larger discussion |
||||
|
* `feature request` - Any issue that requests a new feature (usually not PRs) |
||||
|
* `good first contribution` - Issues suitable for newcomers to process |
||||
|
|
||||
|
* `semver-{minor,major}` |
||||
|
* be conservative – that is, if a change has the remote *chance* of breaking something, go for semver-major |
||||
|
* when adding a semver label, add a comment explaining why you're adding it |
||||
|
* minor vs. patch: roughly: "does it add a new method / does it add a new section to the docs" |
||||
|
* major vs. everything else: run last versions tests against this version, if they pass, **probably** minor or patch |
||||
|
* A breaking change helper ([full source](https://gist.github.com/chrisdickinson/ba532fa0e4e243fb7b44)): |
||||
|
``` |
||||
|
git checkout $(git show -s --pretty='%T' $(git show-ref -d $(git describe --abbrev=0) | tail -n1 | awk '{print $1}')) -- test; make -j8 test |
||||
|
``` |
||||
|
|
||||
|
|
||||
|
### Other Labels |
||||
|
|
||||
|
* Operating system labels |
||||
|
* `os x`, `windows`, `solaris` |
||||
|
* No linux, linux is the implied default |
||||
|
* Architecture labels |
||||
|
* `arm`, `mips` |
||||
|
* No x86{_64}, since that is the implied default |
||||
|
* `lts-agenda`, `lts-watch-v*` |
||||
|
* tag things that should be discussed to go into LTS or should go into a specific LTS branch |
||||
|
* (usually only semver-patch things) |
||||
|
* will come more naturally over time |
||||
|
|
||||
|
|
||||
|
## Updating Node.js from Upstream |
||||
|
|
||||
|
* `git remote add upstream git://github.com/nodejs/node.git` |
||||
|
|
||||
|
to update from nodejs/node: |
||||
|
* `git checkout master` |
||||
|
* `git remote update -p` OR `git fetch --all` (I prefer the former) |
||||
|
* `git merge --ff-only upstream/master` (or `REMOTENAME/BRANCH`) |
||||
|
|
||||
|
|
||||
|
## If `git am` fails |
||||
|
|
||||
|
* if `git am` fails – use `git am --abort` |
||||
|
* this usually means the PR needs updated |
||||
|
* prefer to make the originating user update the code, since they have it fresh in mind |
||||
|
* first, reattempt with `git am -3` (3-way merge)` |
||||
|
* if `-3` still fails, and you need to get it merged: |
||||
|
* `git fetch origin pull/N/head:pr-N && git checkout pr-N && git rebase master` |
||||
|
|
||||
|
|
||||
|
## best practices |
||||
|
|
||||
|
* commit often, out to your github fork (origin), open a PR |
||||
|
* when making PRs make sure to spend time on the description: |
||||
|
* every moment you spend writing a good description quarters the amount of time it takes to understand your code. |
||||
|
* usually prefer to only squash at the *end* of your work, depends on the change |
||||
@ -0,0 +1,199 @@ |
|||||
|
## pre-setup |
||||
|
|
||||
|
Ensure everyone is added to https://github.com/orgs/nodejs/teams/collaborators |
||||
|
|
||||
|
|
||||
|
## onboarding to nodejs |
||||
|
|
||||
|
### intros |
||||
|
|
||||
|
|
||||
|
### **thank you** for doing this |
||||
|
|
||||
|
* going to cover four things: |
||||
|
* local setup |
||||
|
* some project goals & values |
||||
|
* issues, labels, and reviewing code |
||||
|
* merging code |
||||
|
|
||||
|
|
||||
|
### setup: |
||||
|
|
||||
|
* notifications setup |
||||
|
* use https://github.com/notifications or set up email |
||||
|
* watching the main repo will flood your inbox, so be prepared |
||||
|
|
||||
|
|
||||
|
* git: |
||||
|
* make sure you have whitespace=fix: `git config --global --add core.whitespace fix` |
||||
|
* usually PR from your own github fork |
||||
|
* [**See "Updating Node.js from Upstream"**](./onboarding-extras.md#updating-nodejs-from-upstream) |
||||
|
* make new branches for all commits you make! |
||||
|
|
||||
|
|
||||
|
* `#node-dev` on `chat.freenode.net` is the best place to interact with the CTC / other collaborators |
||||
|
|
||||
|
|
||||
|
### a little deeper about the project |
||||
|
|
||||
|
* collaborators are effectively part owners |
||||
|
* the project has the goals of its contributors |
||||
|
|
||||
|
|
||||
|
* but, there are some higher-level goals and values |
||||
|
* not everything belongs in core (if it can be done reasonably in userland, let it stay in userland) |
||||
|
* empathy towards users matters (this is in part why we onboard people) |
||||
|
* generally: try to be nice to people |
||||
|
|
||||
|
|
||||
|
### managing the issue tracker |
||||
|
|
||||
|
* you have (mostly) free rein – don't hesitate to close an issue if you are confident that it should be closed |
||||
|
* this will come more naturally over time |
||||
|
* IMPORTANT: be nice about closing issues, let people know why, and that issues and PRs can be reopened if necessary |
||||
|
* Still need to follow the Code of Conduct. |
||||
|
|
||||
|
|
||||
|
* labels: |
||||
|
* generally sort issues by a concept of "subsystem" so that we know what part(s) of the codebase it touches, though there are also other useful labels. |
||||
|
* [**See "Labels"**](./onboarding-extras.md#labels) |
||||
|
* `ctc-agenda` if a topic is controversial or isn't coming to a conclusion after an extended time. |
||||
|
* `semver-{minor,major}`: |
||||
|
* be conservative – that is, if a change has the remote *chance* of breaking something, go for `semver-major` |
||||
|
* when adding a semver label, add a comment explaining why you're adding it |
||||
|
* it's cached locally in your brain at that moment! |
||||
|
|
||||
|
|
||||
|
* Notifying humans |
||||
|
* [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) |
||||
|
* will also come more naturally over time |
||||
|
|
||||
|
|
||||
|
* reviewing: |
||||
|
* primary goal is for the codebase to improve |
||||
|
* secondary (but not far off) is for the person submitting code to succeed |
||||
|
* helps grow the community |
||||
|
* and draws new people into the project |
||||
|
* Review a bit at a time. It is **very important** to not overwhelm newer people. |
||||
|
* it is tempting to micro-optimize / make everything about relative perf, |
||||
|
don't succumb to that temptation. we change v8 a lot more often now, contortions |
||||
|
that are zippy today may be unnecessary in the future |
||||
|
* be aware: your opinion carries a lot of weight! |
||||
|
* nits are fine, but try to avoid stalling the PR |
||||
|
* note that they are nits when you comment |
||||
|
* if they really are stalling nits, fix them yourself on merge (but try to let PR authors know they can fix these) |
||||
|
* improvement doesn't have to come all at once |
||||
|
* minimum wait for comments time |
||||
|
* There is a minimum waiting time which we try to respect for non-trivial changes, so that people who may have important input in such a distributed project are able to respond. |
||||
|
* It may help to set time limits and expectations: |
||||
|
* the collaborators are very distributed so it is unlikely that they will be looking at stuff the same time as you are. |
||||
|
* before merging code: give folks at least one working day to respond: "If no one objects, tomorrow at <time> I'll merge this in." |
||||
|
* please always either specify your timezone, or use UTC time |
||||
|
* set reminders |
||||
|
* check in on the code every once in a while (set reminders!) |
||||
|
* 48 hours for non-trivial changes, and 72 hours on weekends. |
||||
|
* if a PR is abandoned, check if they'd mind if you took it over (especially if it just has nits left) |
||||
|
* you have the power to `LGTM` another collaborator or TSC / CTC members' work |
||||
|
|
||||
|
|
||||
|
* what belongs in node: |
||||
|
* opinions vary, but I find the following helpful: |
||||
|
* if node itself needs it (due to historic reasons), then it belongs in node |
||||
|
* that is to say, url is there because of http, freelist is there because of http, et al |
||||
|
* also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) |
||||
|
|
||||
|
|
||||
|
* CI testing: |
||||
|
* lives here: https://ci.nodejs.org/ |
||||
|
* not automatically run - some of the platforms we test do not have full sandboxing support so we need to ensure what we run on it isn't potentially malicious |
||||
|
* make sure to log in – we use github authentication so it should be seamless |
||||
|
* go to "node-test-pull-request" and "Build with parameters" |
||||
|
* fill in the pull request number without the `#`, and check the verification that you have reviewed the code for potential malice |
||||
|
* The other options shouldn't need to be adjusted in most cases. |
||||
|
* link to the CI run in the PR by commenting "CI: <ci run link>" |
||||
|
|
||||
|
|
||||
|
### process for getting code in: |
||||
|
|
||||
|
* the collaborator guide is a great resource: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto |
||||
|
|
||||
|
|
||||
|
* 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 |
||||
|
|
||||
|
|
||||
|
* be sure to wait before merging non-trivial changes |
||||
|
* 48 hours for non-trivial changes, and 72 hours on weekends. |
||||
|
|
||||
|
|
||||
|
* **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 |
||||
|
* `#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). |
||||
|
|
||||
|
|
||||
|
### Landing PRs |
||||
|
|
||||
|
* Please never use GitHub's green "Merge Pull Request" button. |
||||
|
* If you do, please force-push removing the merge. |
||||
|
|
||||
|
Update your `master` branch (or whichever branch you are landing on, almost always `master`) |
||||
|
|
||||
|
* [**See "Updating Node.js from Upstream"**](./onboarding-extras.md#updating-nodejs-from-upstream) |
||||
|
|
||||
|
Landing a PR |
||||
|
|
||||
|
* if it all looks good, `curl -L 'url-of-pr.patch' | git am` |
||||
|
* `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.) |
||||
|
* Amend the commit description |
||||
|
* commits should follow `subsystem[,subsystem]: small description\n\nbig description\n\n<metadata>` |
||||
|
* first line 50 columns, all others 72 |
||||
|
* add metadata: |
||||
|
* `Fixes: <full-issue-url>` |
||||
|
* `Reviewed-By: human <email>` |
||||
|
* Easiest to use `git log` then do a search |
||||
|
* (`/Name` + `enter` (+ `n` as much as you need to) in vim) |
||||
|
* `PR-URL: <full-pr-url>` |
||||
|
* `git push upstream master` |
||||
|
* close the original PR with "Landed in `<commit hash>`". |
||||
|
|
||||
|
|
||||
|
### exercise: make PRs adding yourselves to the README. |
||||
|
|
||||
|
* Example: https://github.com/nodejs/node/commit/7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 |
||||
|
* to see full URL: `git log 7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 -1` |
||||
|
* Collaborators in alphabetical order by username |
||||
|
* Label your pull request with the `doc` subsystem label |
||||
|
* If you would like to run CI on your PR, feel free to |
||||
|
* Make sure to added the `PR-URL: <full-pr-url>`! |
||||
|
|
||||
|
|
||||
|
### final notes: |
||||
|
|
||||
|
* don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!) |
||||
|
* very few (no?) mistakes are unrecoverable |
||||
|
* the existing node committers trust you and are grateful for your help! |
||||
|
* other repos: |
||||
|
* https://github.com/nodejs/dev-policy |
||||
|
* https://github.com/nodejs/NG |
||||
|
* https://github.com/nodejs/api |
||||
|
* https://github.com/nodejs/build |
||||
|
* https://github.com/nodejs/docs |
||||
|
* https://github.com/nodejs/nodejs.org |
||||
|
* https://github.com/nodejs/readable-stream |
||||
|
* https://github.com/nodejs/LTS |
||||
Loading…
Reference in new issue