Commit 9794abb
doc: add onboarding resources
PR-URL: #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>1 parent 31e39fb commit 9794abb
2 files changed
Lines changed: 304 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,105 @@ | |||
| 1 | + ## Who to CC in issues | ||
| 2 | + | ||
| 3 | + * `lib/buffer`: @trevnorris | ||
| 4 | + * `lib/child_process`: @cjihrig, @bnoordhuis, @piscisaereus | ||
| 5 | + * `lib/cluster`: @cjihrig, @bnoordhuis, @piscisaereus | ||
| 6 | + * `lib/{crypto,tls,https}`: @indutny, @shigeki, @nodejs/crypto | ||
| 7 | + * `lib/domains`: @misterdjules | ||
| 8 | + * `lib/{_}http{*}`: @indutny, @bnoordhuis, @nodejs/http | ||
| 9 | + * `lib/net`: @indutny, @bnoordhuis, @piscisaereus, @chrisdickinson, @nodejs/streams | ||
| 10 | + * `lib/{_}stream{s|*}`: @nodejs/streams | ||
| 11 | + * `lib/repl`: @fishrock123 | ||
| 12 | + * `lib/timers`: @fishrock123, @misterdjules | ||
| 13 | + * `lib/zlib`: @indutny, @bnoordhuis | ||
| 14 | + | ||
| 15 | + * `src/async-wrap.*`: @trevnorris | ||
| 16 | + * `src/node_crypto.*`: @indutny, @shigeki, @nodejs/crypto | ||
| 17 | + | ||
| 18 | + * `test/*`: @nodejs/testing, @trott | ||
| 19 | + | ||
| 20 | + * `tools/eslint`, `.eslintrc`: @silverwind, @trott | ||
| 21 | + | ||
| 22 | + * upgrading v8: @bnoordhuis / @targos / @ofrobots | ||
| 23 | + * upgrading npm: @thealphanerd, @fishrock123 | ||
| 24 | + | ||
| 25 | + | ||
| 26 | + When things need extra attention, are controversial, or `semver-major`: @nodejs/ctc | ||
| 27 | + | ||
| 28 | + If you cannot find who to cc for a file, `git shortlog -n -s <file>` may help. | ||
| 29 | + | ||
| 30 | + | ||
| 31 | + ## Labels | ||
| 32 | + | ||
| 33 | + ### By Subsystem | ||
| 34 | + | ||
| 35 | + We generally sort issues by a concept of "subsystem" so that we know what part(s) of the codebase it touches. | ||
| 36 | + | ||
| 37 | + **Subsystems generally are**: | ||
| 38 | + | ||
| 39 | + * `lib/*.js` | ||
| 40 | + * `doc`, `build`, `tools`, `test`, `deps`, `lib / src` (special), and there may be others. | ||
| 41 | + * `meta` for anything non-code (process) related | ||
| 42 | + | ||
| 43 | + There may be more than one subsystem valid for any particular issue / PR. | ||
| 44 | + | ||
| 45 | + | ||
| 46 | + ### General | ||
| 47 | + | ||
| 48 | + Please use these when possible / appropriate | ||
| 49 | + | ||
| 50 | + * `confirmed-bug` - Bugs you have verified exist | ||
| 51 | + * `discuss` - Things that need larger discussion | ||
| 52 | + * `feature request` - Any issue that requests a new feature (usually not PRs) | ||
| 53 | + * `good first contribution` - Issues suitable for newcomers to process | ||
| 54 | + | ||
| 55 | + * `semver-{minor,major}` | ||
| 56 | + * be conservative – that is, if a change has the remote *chance* of breaking something, go for semver-major | ||
| 57 | + * when adding a semver label, add a comment explaining why you're adding it | ||
| 58 | + * minor vs. patch: roughly: "does it add a new method / does it add a new section to the docs" | ||
| 59 | + * major vs. everything else: run last versions tests against this version, if they pass, **probably** minor or patch | ||
| 60 | + * A breaking change helper ([full source](https://gist.github.com/chrisdickinson/ba532fa0e4e243fb7b44)): | ||
| 61 | + ``` | ||
| 62 | + git checkout $(git show -s --pretty='%T' $(git show-ref -d $(git describe --abbrev=0) | tail -n1 | awk '{print $1}')) -- test; make -j8 test | ||
| 63 | + ``` | ||
| 64 | + | ||
| 65 | + | ||
| 66 | + ### Other Labels | ||
| 67 | + | ||
| 68 | + * Operating system labels | ||
| 69 | + * `os x`, `windows`, `solaris` | ||
| 70 | + * No linux, linux is the implied default | ||
| 71 | + * Architecture labels | ||
| 72 | + * `arm`, `mips` | ||
| 73 | + * No x86{_64}, since that is the implied default | ||
| 74 | + * `lts-agenda`, `lts-watch-v*` | ||
| 75 | + * tag things that should be discussed to go into LTS or should go into a specific LTS branch | ||
| 76 | + * (usually only semver-patch things) | ||
| 77 | + * will come more naturally over time | ||
| 78 | + | ||
| 79 | + | ||
| 80 | + ## Updating Node.js from Upstream | ||
| 81 | + | ||
| 82 | + * `git remote add upstream git://github.com/nodejs/node.git` | ||
| 83 | + | ||
| 84 | + to update from nodejs/node: | ||
| 85 | + * `git checkout master` | ||
| 86 | + * `git remote update -p` OR `git fetch --all` (I prefer the former) | ||
| 87 | + * `git merge --ff-only upstream/master` (or `REMOTENAME/BRANCH`) | ||
| 88 | + | ||
| 89 | + | ||
| 90 | + ## If `git am` fails | ||
| 91 | + | ||
| 92 | + * if `git am` fails – use `git am --abort` | ||
| 93 | + * this usually means the PR needs updated | ||
| 94 | + * prefer to make the originating user update the code, since they have it fresh in mind | ||
| 95 | + * first, reattempt with `git am -3` (3-way merge)` | ||
| 96 | + * if `-3` still fails, and you need to get it merged: | ||
| 97 | + * `git fetch origin pull/N/head:pr-N && git checkout pr-N && git rebase master` | ||
| 98 | + | ||
| 99 | + | ||
| 100 | + ## best practices | ||
| 101 | + | ||
| 102 | + * commit often, out to your github fork (origin), open a PR | ||
| 103 | + * when making PRs make sure to spend time on the description: | ||
| 104 | + * every moment you spend writing a good description quarters the amount of time it takes to understand your code. | ||
| 105 | + * usually prefer to only squash at the *end* of your work, depends on the change | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,199 @@ | |||
| 1 | + ## pre-setup | ||
| 2 | + | ||
| 3 | + Ensure everyone is added to https://github.com/orgs/nodejs/teams/collaborators | ||
| 4 | + | ||
| 5 | + | ||
| 6 | + ## onboarding to nodejs | ||
| 7 | + | ||
| 8 | + ### intros | ||
| 9 | + | ||
| 10 | + | ||
| 11 | + ### **thank you** for doing this | ||
| 12 | + | ||
| 13 | + * going to cover four things: | ||
| 14 | + * local setup | ||
| 15 | + * some project goals & values | ||
| 16 | + * issues, labels, and reviewing code | ||
| 17 | + * merging code | ||
| 18 | + | ||
| 19 | + | ||
| 20 | + ### setup: | ||
| 21 | + | ||
| 22 | + * notifications setup | ||
| 23 | + * use https://github.com/notifications or set up email | ||
| 24 | + * watching the main repo will flood your inbox, so be prepared | ||
| 25 | + | ||
| 26 | + | ||
| 27 | + * git: | ||
| 28 | + * make sure you have whitespace=fix: `git config --global --add core.whitespace fix` | ||
| 29 | + * usually PR from your own github fork | ||
| 30 | + * [**See "Updating Node.js from Upstream"**](./onboarding-extras.md#updating-nodejs-from-upstream) | ||
| 31 | + * make new branches for all commits you make! | ||
| 32 | + | ||
| 33 | + | ||
| 34 | + * `#node-dev` on `chat.freenode.net` is the best place to interact with the CTC / other collaborators | ||
| 35 | + | ||
| 36 | + | ||
| 37 | + ### a little deeper about the project | ||
| 38 | + | ||
| 39 | + * collaborators are effectively part owners | ||
| 40 | + * the project has the goals of its contributors | ||
| 41 | + | ||
| 42 | + | ||
| 43 | + * but, there are some higher-level goals and values | ||
| 44 | + * not everything belongs in core (if it can be done reasonably in userland, let it stay in userland) | ||
| 45 | + * empathy towards users matters (this is in part why we onboard people) | ||
| 46 | + * generally: try to be nice to people | ||
| 47 | + | ||
| 48 | + | ||
| 49 | + ### managing the issue tracker | ||
| 50 | + | ||
| 51 | + * you have (mostly) free rein – don't hesitate to close an issue if you are confident that it should be closed | ||
| 52 | + * this will come more naturally over time | ||
| 53 | + * IMPORTANT: be nice about closing issues, let people know why, and that issues and PRs can be reopened if necessary | ||
| 54 | + * Still need to follow the Code of Conduct. | ||
| 55 | + | ||
| 56 | + | ||
| 57 | + * labels: | ||
| 58 | + * 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. | ||
| 59 | + * [**See "Labels"**](./onboarding-extras.md#labels) | ||
| 60 | + * `ctc-agenda` if a topic is controversial or isn't coming to a conclusion after an extended time. | ||
| 61 | + * `semver-{minor,major}`: | ||
| 62 | + * be conservative – that is, if a change has the remote *chance* of breaking something, go for `semver-major` | ||
| 63 | + * when adding a semver label, add a comment explaining why you're adding it | ||
| 64 | + * it's cached locally in your brain at that moment! | ||
| 65 | + | ||
| 66 | + | ||
| 67 | + * Notifying humans | ||
| 68 | + * [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) | ||
| 69 | + * will also come more naturally over time | ||
| 70 | + | ||
| 71 | + | ||
| 72 | + * reviewing: | ||
| 73 | + * primary goal is for the codebase to improve | ||
| 74 | + * secondary (but not far off) is for the person submitting code to succeed | ||
| 75 | + * helps grow the community | ||
| 76 | + * and draws new people into the project | ||
| 77 | + * Review a bit at a time. It is **very important** to not overwhelm newer people. | ||
| 78 | + * it is tempting to micro-optimize / make everything about relative perf, | ||
| 79 | + don't succumb to that temptation. we change v8 a lot more often now, contortions | ||
| 80 | + that are zippy today may be unnecessary in the future | ||
| 81 | + * be aware: your opinion carries a lot of weight! | ||
| 82 | + * nits are fine, but try to avoid stalling the PR | ||
| 83 | + * note that they are nits when you comment | ||
| 84 | + * if they really are stalling nits, fix them yourself on merge (but try to let PR authors know they can fix these) | ||
| 85 | + * improvement doesn't have to come all at once | ||
| 86 | + * minimum wait for comments time | ||
| 87 | + * 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. | ||
| 88 | + * It may help to set time limits and expectations: | ||
| 89 | + * the collaborators are very distributed so it is unlikely that they will be looking at stuff the same time as you are. | ||
| 90 | + * before merging code: give folks at least one working day to respond: "If no one objects, tomorrow at <time> I'll merge this in." | ||
| 91 | + * please always either specify your timezone, or use UTC time | ||
| 92 | + * set reminders | ||
| 93 | + * check in on the code every once in a while (set reminders!) | ||
| 94 | + * 48 hours for non-trivial changes, and 72 hours on weekends. | ||
| 95 | + * if a PR is abandoned, check if they'd mind if you took it over (especially if it just has nits left) | ||
| 96 | + * you have the power to `LGTM` another collaborator or TSC / CTC members' work | ||
| 97 | + | ||
| 98 | + | ||
| 99 | + * what belongs in node: | ||
| 100 | + * opinions vary, but I find the following helpful: | ||
| 101 | + * if node itself needs it (due to historic reasons), then it belongs in node | ||
| 102 | + * that is to say, url is there because of http, freelist is there because of http, et al | ||
| 103 | + * also, things that cannot be done outside of core, or only with significant pain (example: async-wrap) | ||
| 104 | + | ||
| 105 | + | ||
| 106 | + * CI testing: | ||
| 107 | + * lives here: https://ci.nodejs.org/ | ||
| 108 | + * 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 | ||
| 109 | + * make sure to log in – we use github authentication so it should be seamless | ||
| 110 | + * go to "node-test-pull-request" and "Build with parameters" | ||
| 111 | + * fill in the pull request number without the `#`, and check the verification that you have reviewed the code for potential malice | ||
| 112 | + * The other options shouldn't need to be adjusted in most cases. | ||
| 113 | + * link to the CI run in the PR by commenting "CI: <ci run link>" | ||
| 114 | + | ||
| 115 | + | ||
| 116 | + ### process for getting code in: | ||
| 117 | + | ||
| 118 | + * the collaborator guide is a great resource: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto | ||
| 119 | + | ||
| 120 | + | ||
| 121 | + * no one (including TSC or CTC members) pushes directly to master without review | ||
| 122 | + * an exception is made for release commits only | ||
| 123 | + | ||
| 124 | + | ||
| 125 | + * one "LGTM" is usually sufficient, except for semver-major changes | ||
| 126 | + * the more the better | ||
| 127 | + * semver-major (breaking) changes must be reviewed in some form by the CTC | ||
| 128 | + | ||
| 129 | + | ||
| 130 | + * be sure to wait before merging non-trivial changes | ||
| 131 | + * 48 hours for non-trivial changes, and 72 hours on weekends. | ||
| 132 | + | ||
| 133 | + | ||
| 134 | + * **make sure to run the PR through CI before merging!** (Except for documentation PRs) | ||
| 135 | + | ||
| 136 | + | ||
| 137 | + * once code is ready to go in: | ||
| 138 | + * [**See "Landing PRs"**](#landing-prs) below | ||
| 139 | + | ||
| 140 | + | ||
| 141 | + * what if something goes wrong? | ||
| 142 | + * ping a CTC member | ||
| 143 | + * `#node-dev` on freenode | ||
| 144 | + * 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 | ||
| 145 | + * Info on PRs that don't like to apply found under [**"If `git am` fails"**](./onboarding-extras.md#if-git-am-fails). | ||
| 146 | + | ||
| 147 | + | ||
| 148 | + ### Landing PRs | ||
| 149 | + | ||
| 150 | + * Please never use GitHub's green "Merge Pull Request" button. | ||
| 151 | + * If you do, please force-push removing the merge. | ||
| 152 | + | ||
| 153 | + Update your `master` branch (or whichever branch you are landing on, almost always `master`) | ||
| 154 | + | ||
| 155 | + * [**See "Updating Node.js from Upstream"**](./onboarding-extras.md#updating-nodejs-from-upstream) | ||
| 156 | + | ||
| 157 | + Landing a PR | ||
| 158 | + | ||
| 159 | + * if it all looks good, `curl -L 'url-of-pr.patch' | git am` | ||
| 160 | + * `git rebase -i upstream/master` | ||
| 161 | + * squash into logical commits if necessary | ||
| 162 | + * `./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.) | ||
| 163 | + * Amend the commit description | ||
| 164 | + * commits should follow `subsystem[,subsystem]: small description\n\nbig description\n\n<metadata>` | ||
| 165 | + * first line 50 columns, all others 72 | ||
| 166 | + * add metadata: | ||
| 167 | + * `Fixes: <full-issue-url>` | ||
| 168 | + * `Reviewed-By: human <email>` | ||
| 169 | + * Easiest to use `git log` then do a search | ||
| 170 | + * (`/Name` + `enter` (+ `n` as much as you need to) in vim) | ||
| 171 | + * `PR-URL: <full-pr-url>` | ||
| 172 | + * `git push upstream master` | ||
| 173 | + * close the original PR with "Landed in `<commit hash>`". | ||
| 174 | + | ||
| 175 | + | ||
| 176 | + ### exercise: make PRs adding yourselves to the README. | ||
| 177 | + | ||
| 178 | + * Example: https://github.com/nodejs/node/commit/7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 | ||
| 179 | + * to see full URL: `git log 7b09aade8468e1c930f36b9c81e6ac2ed5bc8732 -1` | ||
| 180 | + * Collaborators in alphabetical order by username | ||
| 181 | + * Label your pull request with the `doc` subsystem label | ||
| 182 | + * If you would like to run CI on your PR, feel free to | ||
| 183 | + * Make sure to added the `PR-URL: <full-pr-url>`! | ||
| 184 | + | ||
| 185 | + | ||
| 186 | + ### final notes: | ||
| 187 | + | ||
| 188 | + * don't worry about making mistakes: everybody makes them, there's a lot to internalize and that takes time (and we recognize that!) | ||
| 189 | + * very few (no?) mistakes are unrecoverable | ||
| 190 | + * the existing node committers trust you and are grateful for your help! | ||
| 191 | + * other repos: | ||
| 192 | + * https://github.com/nodejs/dev-policy | ||
| 193 | + * https://github.com/nodejs/NG | ||
| 194 | + * https://github.com/nodejs/api | ||
| 195 | + * https://github.com/nodejs/build | ||
| 196 | + * https://github.com/nodejs/docs | ||
| 197 | + * https://github.com/nodejs/nodejs.org | ||
| 198 | + * https://github.com/nodejs/readable-stream | ||
| 199 | + * https://github.com/nodejs/LTS | ||
0 commit comments