mirrors/nodejs
mirrors/nodejs
0
0
Fork 0
mirror of https://github.com/nodejs/node.git synced 2024-11-29 23:16:30 +01:00
Code Issues Releases Activity
ae178838eb
nodejs/doc/onboarding.md
Jeremiah Senkpiel 7a3ed98229 doc: add onboarding resources 
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>
2016-03-14 11:35:04 -04:00

8.5 KiB
Raw Blame History

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

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

  • 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
          • 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: "

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:

  • 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".

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)

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: 7b09aade84
    • 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: