Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: reorganize COLLABORATOR_GUIDE.md #15710

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 32 additions & 28 deletions COLLABORATOR_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -371,60 +371,45 @@ The TSC should serve as the final arbiter where required.
* If more than one author has contributed to the PR, keep the most recent
author when squashing.

Always modify the original commit message to include additional meta
information regarding the change process:

- A `PR-URL:` line that references the *full* GitHub URL of the original
pull request being merged so it's easy to trace a commit back to the
conversation that led up to that change.
- A `Fixes: X` line, where _X_ either includes the *full* GitHub URL
for an issue, and/or the hash and commit message if the commit fixes
a bug in a previous commit. Multiple `Fixes:` lines may be added if
appropriate.
- A `Refs:` line referencing a URL for any relevant background.
- A `Reviewed-By: Name <email>` line for yourself and any
other Collaborators who have reviewed the change.
- Useful for @mentions / contact list if something goes wrong in the PR.
- Protects against the assumption that GitHub will be around forever.

Review the commit message to ensure that it adheres to the guidelines outlined
in the [contributing](./CONTRIBUTING.md#commit-message-guidelines) guide.

Add all necessary [metadata](#metadata) to commit messages before landing.

See the commit log for examples such as
[this one](https://github.com/nodejs/node/commit/b636ba8186) if unsure
exactly how to format your commit messages.

Additionally:
- Double check PRs to make sure the person's _full name_ and email
address are correct before merging.
- Except when updating dependencies, all commits should be self
contained (meaning every commit should pass all tests). This makes
it much easier when bisecting to find a breaking change.
- All commits should be self-contained (meaning every commit should pass all
tests). This makes it much easier when bisecting to find a breaking change.

### Technical HOWTO

Clear any `am`/`rebase` that may already be underway.
Clear any `am`/`rebase` that may already be underway:

```text
$ git am --abort
$ git rebase --abort
```

Checkout proper target branch
Checkout proper target branch:

```text
$ git checkout master
```

Update the tree (assumes your repo is set up as detailed in
[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork))
[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)):

```text
$ git fetch upstream
$ git merge --ff-only upstream/master
```

Apply external patches
Apply external patches:

```text
$ curl -L https://github.com/nodejs/node/pull/xxx.patch | git am --whitespace=fix
Expand All @@ -442,21 +427,19 @@ against the original PR carefully and build/test on at least one platform
before landing. If the 3-way merge fails, then it is most likely that a conflicting
PR has landed since the CI run and you will have to ask the author to rebase.

Check and re-review the changes
Check and re-review the changes:

```text
$ git diff upstream/master
```

Check number of commits and commit messages
Check number of commits and commit messages:

```text
$ git log upstream/master...master
```

If there are multiple commits that relate to the same feature or
one with a feature and separate with a test for that feature,
you'll need to use `squash` or `fixup`:
Squash commits and add metadata:

```text
$ git rebase -i upstream/master
Expand Down Expand Up @@ -512,9 +495,29 @@ Save the file and close the editor. You'll be asked to enter a new
commit message for that commit. This is a good moment to fix incorrect
commit logs, ensure that they are properly formatted, and add
`Reviewed-By` lines.

* The commit message text must conform to the
[commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines).

<a name="metadata"></a>
* Modify the original commit message to include additional metadata regarding
the change process. (If you use Chrome or Edge, [`node-review`][] fetches
the metadata for you.)

* Required: A `PR-URL:` line that references the *full* GitHub URL of the
original pull request being merged so it's easy to trace a commit back to
the conversation that led up to that change.
* Optional: A `Fixes: X` line, where _X_ either includes the *full* GitHub URL
for an issue, and/or the hash and commit message if the commit fixes
a bug in a previous commit. Multiple `Fixes:` lines may be added if
appropriate.
* Optional: One or more `Refs:` lines referencing a URL for any relevant
background.
* Required: A `Reviewed-By: Name <email>` line for yourself and any
other Collaborators who have reviewed the change.
* Useful for @mentions / contact list if something goes wrong in the PR.
* Protects against the assumption that GitHub will be around forever.

Run tests (`make -j4 test` or `vcbuild test`). Even though there was a
successful continuous integration run, other changes may have landed on master
since then, so running the tests one last time locally is a good practice.
Expand Down Expand Up @@ -678,3 +681,4 @@ LTS working group and the Release team.
[Stability Index]: doc/api/documentation.md#stability-index
[Enhancement Proposal]: https://github.com/nodejs/node-eps
[git-username]: https://help.github.com/articles/setting-your-username-in-git/
[`node-review`]: https://github.com/evanlucas/node-review
2 changes: 1 addition & 1 deletion doc/onboarding.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ onboarding session.
* After one or two approvals, land the PR.
* Be sure to add the `PR-URL: <full-pr-url>` and appropriate `Reviewed-By:` metadata!
* [`core-validate-commit`][] helps a lot with this – install and use it if you can!
* If you use Chrome, [`node-review`][] fetches the metadata for you
* If you use Chrome or Edge, [`node-review`][] fetches the metadata for you.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra! Extra! Read all about it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New CLI tool node-core-utils fetches the needed meta data from the command line.


## Final notes

Expand Down