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