Skip to content

Commit 9794abb

Browse files
Fishrock123rvagg
authored andcommitted
doc: add onboarding resources
PR-URL: #3726 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chris Dickinson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
1 parent 31e39fb commit 9794abb

File tree

2 files changed

+304
-0
lines changed

2 files changed

+304
-0
lines changed

doc/onboarding-extras.md

+105
Original file line numberDiff line numberDiff 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

doc/onboarding.md

+199
Original file line numberDiff line numberDiff 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

Comments
 (0)