doc: update collaborator guide #19116
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,16 @@ | ||
| # Node.js Collaborator Guide | ||
|
|
||
| ## Contents | ||
|
|
||
| * [Issues and Pull Requests](#issues-and-pull-requests) | ||
| - [Managing Issues and Pull Requests](#managing-issues-and-pull-requests) | ||
| - [Welcoming First-Time Contributors](#welcoming-first-time-contributors) | ||
| - [Closing Issues and Pull Requests](#closing-issues-and-pull-requests) | ||
| - [Ready to land pull requests](#ready-to-land-pull-requests) | ||
| - [Handling own pull requests](#handling-own-pull-requests) | ||
| * [Accepting Modifications](#accepting-modifications) | ||
| - [Code Reviews](#code-reviews) | ||
| - [Consensus Seeking](#consensus-seeking) | ||
| - [Waiting for Approvals](#waiting-for-approvals) | ||
| - [Testing and CI](#testing-and-ci) | ||
| - [Useful CI Jobs](#useful-ci-jobs) | ||
|
|
@@ -45,13 +48,12 @@ understand the project governance model as outlined in | |
|
|
||
| ### Managing Issues and Pull Requests | ||
|
|
||
| Collaborators should take full responsibility for managing issues and pull | ||
| requests they feel qualified to handle. Just make sure this is done while being | ||
| mindful of these guidelines, the opinions of other Collaborators and guidance of | ||
|
There was a problem hiding this comment. Nit: add a comma after |
||
| the [TSC][]. They may also notify other qualified parties for more input on an | ||
| issue or a pull request. [See "Who to CC in | ||
| issues"](./doc/onboarding-extras.md#who-to-cc-in-issues) | ||
|
|
||
| ### Welcoming First-Time Contributors | ||
|
|
||
|
|
@@ -75,47 +77,86 @@ Collaborators or additional evidence that the issue has relevance, the | |
| issue may be closed. Remember that issues can always be re-opened if | ||
| necessary. | ||
|
|
||
| ### Ready to land pull requests | ||
|
|
||
| A pull request that is still awaiting the minimum review time is considered | ||
|
There was a problem hiding this comment. Nit: There was a problem hiding this comment. In this sentence my intention was to explain what There was a problem hiding this comment. It's a nit; if you think it's better the way it is, then that works for me. |
||
| `ready` as soon as the CI got kicked off, it got at least one approval and | ||
|
There was a problem hiding this comment. Nit: Change first Nit: Add a comma after |
||
| it has no outstanding review comments. Please always make sure to add the | ||
| appropriate `ready` label to the PR in that case and remove it again as soon | ||
| as that condition is not met anymore. | ||
|
|
||
| ### Handling own pull requests | ||
|
|
||
| If you as a collaborator open a pull request, please always start a CI right | ||
|
|
||
| after (see [testing and CI](#testing-and-ci) for further information on how to | ||
| do that) and post the link to it as well. Please also make sure that you start a | ||
| new CI after each update (due to e.g., a change request in a review or due to | ||
| rebasing). | ||
|
There was a problem hiding this comment. I'm not 100% sure starting a CI right after opening is the best way to go. I often wait until there are a few reviews to see if I'm going to have to make any updates. There was a problem hiding this comment. I would like to keep it this way until we get that part automated. Do you feel strongly about it? See the former discussion for the reasons, e.g., #19116 (comment). There was a problem hiding this comment. @BridgeAR Can we hold off landing this before we have consensus here? Or, alternatively, could you reword this as a suggestion? (That is a general feeling I have about this PR, btw: I don’t think we should impose our own workflows on other people.) There was a problem hiding this comment. I personally am not a huge fan about starting a CI always either ^^. The only reason why I think it is good, is that it improves the handling of the PRs as far as I see it. This is, by all means, not meant to impose a workflow on someone. Even with the current wording I do not see it as something mandatory, just as something that is really nice to have. I am going to tone down a wording a bit. |
||
|
|
||
| As soon as the PR is ready to land, please go ahead and do so on your own. | ||
| Landing own pull requests distributes the work load for each collaborator | ||
|
There was a problem hiding this comment. Nit: Missing There was a problem hiding this comment. Is |
||
| equally. If it is still awaiting the | ||
| [minimum time to land](#waiting-for-approvals), please add the `ready` label to | ||
| it so it is obvious that the PR can land as soon as the time ends. | ||
|
There was a problem hiding this comment. This sentence assumes that a "ready to land" means it has a passing CI. However, in the previous paragraph it's defined as "a PR with CI started", which definitely does not meet our criteria to landing them (we want CI passing, right?). |
||
|
|
||
| ## Accepting Modifications | ||
|
|
||
| All modifications to the Node.js code and documentation should be performed via | ||
| GitHub pull requests, including modifications by Collaborators and TSC members. | ||
| A pull request must be reviewed, and must also be tested with CI, before being | ||
| landed into the codebase. There may be exception to the latter. | ||
|
There was a problem hiding this comment. "There may be exception to the latter". When? Clarity would be useful here. |
||
|
|
||
| ### Code Reviews | ||
|
|
||
| All pull requests must be reviewed and accepted by a Collaborator with | ||
| sufficient expertise who is able to take full responsibility for the | ||
| change. In the case of pull requests proposed by an existing | ||
| Collaborator, an additional Collaborator is required for sign-off. | ||
|
|
||
| In some cases, it may be necessary to summon a GitHub team to a pull request for | ||
| review by @-mention. | ||
| [See "Who to CC in issues"](./doc/onboarding-extras.md#who-to-cc-in-issues). | ||
|
|
||
| If you are unsure about the modification and are not prepared to take | ||
| full responsibility for the change, defer to another Collaborator. | ||
|
|
||
| If you are the first collaborator to approve a pull request that has no CI yet, | ||
|
|
||
| please start one (see [testing and CI](#testing-and-ci) for further information | ||
| on how to do that) and post the link to the CI in the PR. Please also start a | ||
| new CI in case a code change was done due to a addressed review comment or a | ||
|
There was a problem hiding this comment. This line is a bit unclear to me. If nothing else: |
||
| rebase. | ||
|
|
||
| In case there are already enough approvals (`LGTM`), a CI run and the PR is open | ||
|
|
||
| longer than the minimum waiting time without any open comments, please do not | ||
| just add another approval. Instead go ahead and land the PR after checking the | ||
|
|
||
| CI outcome. | ||
|
|
||
| ### Consensus Seeking | ||
|
|
||
| If there is no disagreement amongst Collaborators, a pull request should be | ||
| landed given appropriate review, a green CI and the minimum | ||
|
|
||
| [waiting time](#waiting-for-approvals) for a PR. If it is still awaiting the | ||
| [minimum time to land](#waiting-for-approvals), please add the `ready` label to | ||
| it so it is obvious that the PR can land as soon as the time ends. | ||
|
|
||
| Where there is discussion amongst Collaborators, consensus should be sought if | ||
| possible. The lack of consensus may indicate the need to elevate discussion to | ||
| the TSC for resolution. | ||
|
|
||
| If any Collaborator objects to a change *without giving any additional | ||
| explanation or context*, and the objecting Collaborator fails to respond to | ||
| explicit requests for explanation or context within a reasonable period of | ||
| time, the objection may be dismissed. Note that this does not apply to | ||
| objections that are explained. | ||
|
|
||
| Note that breaking changes (that is, pull requests that require an increase in | ||
| the major version number, known as `semver-major` changes) must be [elevated for | ||
| review by the TSC](#involving-the-tsc). This does not necessarily mean that the | ||
| PR must be put onto the TSC meeting agenda. If multiple TSC members approve | ||
| (`LGTM`) the PR and no Collaborators oppose the PR, it should be landed. Where | ||
| there is disagreement among TSC members or objections from one or more | ||
| Collaborators, `semver-major` pull requests should be put on the TSC meeting | ||
|
There was a problem hiding this comment. Suggestion: |
||
| agenda. | ||
|
|
||
| #### Helpful resources | ||
|
|
||
|
|
@@ -147,10 +188,10 @@ CI testing is done. | |
| All bugfixes require a test case which demonstrates the defect. The | ||
| test should *fail* before the change, and *pass* after the change. | ||
|
|
||
| All pull requests that modify executable code should also include a test case | ||
| and be subjected to continuous integration tests on the | ||
| [project CI server](https://ci.nodejs.org/). The pull request should have a CI | ||
| status indicator if possible. | ||
|
|
||
| #### Useful CI Jobs | ||
|
|
||
|
|
@@ -262,7 +303,7 @@ backward-incompatible way) without a deprecation. | |
| Exceptions to this rule may be made in the following cases: | ||
|
|
||
| * Adding or removing errors thrown or reported by a Public API; | ||
| * Changing error messages for errors without error code; | ||
| * Altering the timing and non-internal side effects of the Public API. | ||
|
|
||
| Such changes *must* be handled as semver-major changes but MAY be landed | ||
|
|
@@ -432,28 +473,33 @@ The TSC should serve as the final arbiter where required. | |
|
|
||
| ## Landing Pull Requests | ||
|
|
||
| 1. Never use GitHub's green ["Merge Pull Request"][] button. Reasons for not | ||
| using the web interface button: | ||
| * The merge method will add an unnecessary merge commit. | ||
| * The squash & merge method has been known to add metadata to the commit | ||
| title (the PR #). | ||
| * If more than one author has contributed to the PR, keep the most recent | ||
|
There was a problem hiding this comment. Is this point a bit confusing? If this is an advice, should we get it out of "Reasons for not using..." part? If this is a green button artifact, should the "keep" become "keeps" and should the culprit method be mentioned? There was a problem hiding this comment. In fact, I'd strictly be in favor of keeping code by authors who are not core collaborators in PRs even if they are not the latest. I think that if there are multiple contributors we should keep commits from all of them (perhaps with the same commit message) There was a problem hiding this comment. It requires judgment, particularly around how significant the contribution is. If someone contributed something significant, squashing commits in a way that removes them as an author should certainly be avoided. That can look like (or can actually be) stealing credit for someone else's work. On the other hand, we do have to make sure that all tests pass with each commit, and sometimes squashing is the most reasonable way to achieve that. There was a problem hiding this comment. I see the point that this is confusing but I would really like to keep this out of the scope of this PR. I did not change this part, so I guess that is fine? There was a problem hiding this comment. Oh, yes, hadn't noticed that this content has not changed and this is just a formatting/white-space change to this part. Yeah, the comments should be addressed in another PR IMO. |
||
| author when squashing. | ||
| 1. Make sure the CI is done and the result is green. If the CI is not green, | ||
| check for flaky tests and infrastructure failures. Please check if those were | ||
| already reported in the appropriate repository ([node][flaky tests] and | ||
| [build](https://github.com/nodejs/build/issues)) or not and open new issues | ||
| in case they are not. If not CI was run or the run is outdated because code | ||
|
|
||
| was pushed after the last run, please first start a new CI and wait for the | ||
| result. If no CI is required, please leave a comment in case non is already | ||
|
|
||
| present. | ||
| 1. Review the commit message to ensure that it adheres to the guidelines | ||
| outlined in the [contributing][] guide. | ||
| 1. 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. | ||
| * 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. | ||
|
|
||
| ### Using `git-node` | ||
|
|
@@ -541,7 +587,7 @@ This will open a screen like this (in the default shell editor): | |
| ```text | ||
| pick 6928fc1 crypto: add feature A | ||
| pick 8120c4c add test for feature A | ||
| pick 51759dc crypto: feature B | ||
| pick 7d6f433 test for feature B | ||
|
|
||
| # Rebase f9456a2..7d6f433 onto f9456a2 | ||
|
|
@@ -569,7 +615,7 @@ previous commit: | |
| ```text | ||
| pick 6928fc1 crypto: add feature A | ||
| fixup 8120c4c add test for feature A | ||
| pick 51759dc crypto: feature B | ||
| fixup 7d6f433 test for feature B | ||
| ``` | ||
|
|
||
|
|
@@ -578,7 +624,7 @@ Replace `pick` with `reword` to change the commit message: | |
| ```text | ||
| reword 6928fc1 crypto: add feature A | ||
| fixup 8120c4c add test for feature A | ||
| reword 51759dc crypto: feature B | ||
| fixup 7d6f433 test for feature B | ||
| ``` | ||
|
|
||
|
|
@@ -655,12 +701,12 @@ hint: See the 'Note about fast-forwards' in 'git push --help' for details. | |
| ``` | ||
|
|
||
| That means a commit has landed since your last rebase against `upstream/master`. | ||
| To fix this pull with rebase from upstream and run the tests again (to make sure | ||
|
|
||
| no interactions between your changes and the new changes cause any problems), | ||
| and push again: | ||
|
|
||
| ```sh | ||
| git pull upstream master --rebase | ||
| make -j4 test | ||
| git push upstream master | ||
| ``` | ||
|
|
@@ -721,8 +767,8 @@ TSC for further discussion. | |
|
|
||
| #### How are LTS Branches Managed? | ||
|
|
||
| There are multiple LTS branches, e.g. `v8.x` and `v6.x`. Each of these is paired | ||
| with a staging branch: `v8.x-staging` and `v6.x-staging`. | ||
|
|
||
| As commits land on the master branch, they are cherry-picked back to each | ||
| staging branch as appropriate. If the commit applies only to the LTS branch, the | ||
|
|
@@ -737,10 +783,10 @@ LTS release. | |
|
|
||
| #### How can I help? | ||
|
|
||
| When you send your pull request, please include information about whether your | ||
| change is breaking. If you think your patch can be backported, please include | ||
| that information in the PR thread or your PR description. For more information | ||
| on backporting, please see the [backporting guide][]. | ||
|
|
||
| Several LTS related issue and PR labels have been provided: | ||
|
|
||
|
|
@@ -769,6 +815,7 @@ release. This process of making a release will be a collaboration between the | |
| LTS working group and the Release team. | ||
|
|
||
| [backporting guide]: doc/guides/backporting-to-release-lines.md | ||
| [contributing]: ./doc/guides/contributing/pull-requests.md#commit-message-guidelines | ||
| [Stability Index]: doc/api/documentation.md#stability-index | ||
| [Enhancement Proposal]: https://github.com/nodejs/node-eps | ||
| [`--pending-deprecation`]: doc/api/cli.md#--pending-deprecation | ||
|
|
@@ -779,3 +826,5 @@ LTS working group and the Release team. | |
| [TSC]: https://github.com/nodejs/TSC | ||
| [node-core-utils-issues]: https://github.com/nodejs/node-core-utils/issues | ||
| [node-core-utils-credentials]: https://github.com/nodejs/node-core-utils#setting-up-credentials | ||
| ["Merge Pull Request"]: https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github | ||
| [flaky tests]: https://github.com/nodejs/node/issues?q=is%3Aopen+is%3Aissue+label%3A%22CI+%2F+flaky+test%22 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Get rid of
Just.