Skip to content

Instantly share code, notes, and snippets.

@Fishrock123

Fishrock123/onboarding.md Secret

Forked from chrisdickinson/gist:b7ec569f1d5fe7104391
Last active November 9, 2015 22:09
Show Gist options
  • Save Fishrock123/8d458a00b3164f472691 to your computer and use it in GitHub Desktop.
Save Fishrock123/8d458a00b3164f472691 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
onboarding.md

onboarding to nodejs

thank you for doing this

  • collaborators are effectively part owners
  • going to cover four things:
    • some project goals & values
    • local setup
    • issues, labels, and reviewing code
    • merging code

a little deeper about the project

  • effectively has the goals of it's contributors
  • but, there are some higher-level goals and values
    • not everything belongs in core (if it can be done reasonably in userland, let is stay in userland)
    • empathy towards users matters (this is in part why we onboard people)

setup:

  • notifications setup
  • CI testing:
    • lives here: https://ci.nodejs.org/
    • make sure to log in – I've sent your emails to rvagg, who will set up an account for you
    • go to "Build with parameters"
    • fill in user, repo, and branch. don't need to touch nodes_subset.
  • git:
    • make sure you have whitespace=fix: git config --global --add core.whitespace fix
    • fork the repo to your user account on github
    • git clone git://github.com/USERNAME/node.git
    • 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)
    • make new branches for all commits you make!
  • #io.js on freenode is the best place to be to interact with the TSC / CTC / other collaborators
    • if you don't have IRC or a bouncer, contact me via email after the meeting and we'll get you set up.

managing the issue tracker

  • you have free reign – 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
    • A bit at a time. It's very important to not overwhelm newer people/

  • subsystems:

    • we generally sort by these.
    • lib/*.js
    • doc, build, tools, test, deps, lib / src (special)

  • cc'ing humans (by subsystem):

    • lib/child_process: cjihrig, bnoordhuis, piscisaereus
    • lib/cluster: cjihrig, bnoordhuis, piscisaereus
    • lib/stream*: nodejs/streams
    • lib/net: indutny, bnoordhuis, piscisaereus, chrisdickinson, nodejs/streams
    • lib/http: indutny, bnoordhuis, nodejs/http
    • lib/vm: domenic
    • lib/buffer: trevnorris
    • src/async-wrap.*: trevnorris
    • lib/crypto,tls,https: indutny, shigeki
    • src/node_crypto.*: indutny, shigeki
    • lib/timers: fishrock123
    • upgrading v8: bnoordhuis / targos / ofrobots
    • IN GENERAL: nodejs/ctc, or git shortlog -n -s <file>

  • labels:

    • tsc-agenda if a topic seems controversial
    • semver-major, semver-minor:

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

      • 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

      • adding affected subsystem, os, + node version labels is good as well!

      • https://gist.github.com/chrisdickinson/ba532fa0e4e243fb7b44 – test commits

        git checkout $(git show -s --pretty='%T' $(git show-ref -d $(git describe --abbrev=0) | tail -n1 | awk '{print $1}')) -- test; make test

  • 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
    • 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
    • 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 I'll merge this in."
        • technically, 48 hours for non-trivial changes, and 72 hours on weekends.
        • set reminders
      • check in on the code every once in a while (set reminders!)
      • if a PR is abandoned, check if they'd mind if you took it over (especially if its just nits left)
    • what belongs in node:
      • opinions vary, but I find the following helpful:
      • if node itself needs it, 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 great pain (i.e. async-wrap)

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 (not even TSC or CTC) pushes directly to master without review
    • best practice: commit often, out to your github fork (origin), open a PR
    • 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.
    • prefer to only squash at the end of your work
    • 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
    • you have the power to LGTM another collaborator or TSC / CTC members' work
    • make sure to run the PR through CI before merging! (Except for documentation PRs)
  • once code is ready to go in:
    • git checkout master && git status
    • if the working directory is clean
    • git remote update -p && git merge --ff-only upstream/master
    • To land a PR
    • if it all looks good, curl -L 'url-of-pr.patch' | git am
    • git rebase -i upstream/master
    • squash into logical commits
    • ./configure && make -j8 test (-j8 builds iojs in parallel with 8 threads. adjust to the number of cores (or processor-level threads) your processor has for best results.)
    • commits should follow subsystem[,subsystem]: small description\n\nbig description\n\n<metadata>
    • first line 50 columns, all others 72
    • add metadata:
      • Fixes: <url>
      • Reviewed-By: human <email>
        • Easiest to use git log then do a search (/QUERY + enter (+ n as much as you need to) in vim)
      • PR-URL: <original-full-pr-url>
    • make test-ci && git push upstream master
    • close the original PR with "Landed in <commit hash>".
  • what if something goes wrong?
    • ping a CTC member
    • 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
      • if you must, git fetch origin pull/N/head:pr-N && git checkout pr-N && git rebase master

exercise: make PRs adding yourselves to the README.

final notes:

Feedback form

https://docs.google.com/forms/d/1wtwhnabArplRSEkerkx9JPA1y3F-aWzGWtFFlL3Fwdg/viewform?usp=send_form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment