From 0988b69a6b0618f66a2bd27770b5d8a545d12d31 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 3 Mar 2018 21:55:30 +0000 Subject: [PATCH 1/6] doc: update collaborator guide This also updates some parts of the onboarding. It is mainly about how to handle pull requests, how and when to start a CI, when to add the `ready` label and some other minor changes. --- COLLABORATOR_GUIDE.md | 185 ++++++++++++++++++++++++++---------------- doc/onboarding.md | 20 +++-- 2 files changed, 129 insertions(+), 76 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index e48e016bf7f0c8..1fecd26e5ddf4f 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -1,13 +1,16 @@ # Node.js Collaborator Guide -**Contents** +## 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 and Consensus Seeking](#code-reviews-and-consensus-seeking) + - [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 feel free to take full responsibility for -managing issues and pull requests they feel qualified to handle, as -long as this is done while being mindful of these guidelines, the -opinions of other Collaborators and guidance of 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) +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 +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 +`ready` as soon as the CI got kicked off, it got at least one approval and +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). + +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 +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. + ## 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 usually -must also be tested with CI, before being landed into the codebase. +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. -### Code Reviews and Consensus Seeking +### 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 qualified Collaborator -or 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) +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 +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. -For non-breaking changes, if there is no disagreement amongst -Collaborators, a pull request may be landed given appropriate review. -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 (see below). - -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 can 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 agenda. +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 +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 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. +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; +* 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 -* Please never use GitHub's green ["Merge Pull Request"](https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github) button. - * If you do, please force-push removing the merge. - * 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 - author when squashing. - -Review the commit message to ensure that it adheres to the guidelines outlined -in the [contributing](./doc/guides/contributing/pull-requests.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. +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 + 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 + +* 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 +* 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 feature B +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 feature B +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 feature B +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, fetch, rebase, run the tests again (to make sure no interactions -between your changes and the new changes cause any problems), and push again: +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 fetch upstream -git rebase upstream/master +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 currently two LTS branches: `v6.x` and `v4.x`. Each of these is paired -with a staging branch: `v6.x-staging` and `v4.x-staging`. +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, consider including information about -whether your change is breaking. If you think your patch can be backported, -please feel free to include that information in the PR thread. For more -information on backporting, please see the [backporting guide][]. +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 diff --git a/doc/onboarding.md b/doc/onboarding.md index a85e493d3c7561..d691c044b37a30 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -86,6 +86,10 @@ onboarding session. `semver-major` label * When adding a `semver-*` label, add a comment explaining why you're adding it. Do it right away so you don't forget! + * Please add the `ready` label for PRs where the CI was just kicked off and + that are good to go (no outstanding review items), pending the CI outcome + and possibly the 48/72 hour waiting rule and the necessary LGs for + semver-major. * [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) * This will come more naturally over time @@ -107,11 +111,11 @@ onboarding session. * The primary goal is for the codebase to improve. * Secondary (but not far off) is for the person submitting code to succeed. A - pull request from a new contributor is an opportunity to grow the community. + pull request from a new contributor is an opportunity to grow the community. * Review a bit at a time. Do not overwhelm new contributors. * It is tempting to micro-optimize. Don't succumb to that temptation. We - change V8 often. Techniques that provide improved performance today may be - unnecessary in the future. + change V8 often. Techniques that provide improved performance today may be + unnecessary in the future. * Be aware: Your opinion carries a lot of weight! * Nits (requests for small changes that are not essential) are fine, but try to avoid stalling the pull request. @@ -122,15 +126,15 @@ onboarding session. by tools but are not, consider implementing the necessary tooling. * 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. + changes so that people who may have important input in such a distributed + project are able to respond. * For non-trivial changes, leave the pull request open for at least 48 hours - (72 hours on a weekend). + (72 hours on a weekend). * If a pull request is abandoned, check if they'd mind if you took it over - (especially if it just has nits left). + (especially if it just has nits left). * Approving a change * Collaborators indicate that they have reviewed and approve of the changes in - a pull request using Github’s approval interface + a pull request using Github’s approval interface * Some people like to comment `LGTM` (“Looks Good To Me”) * You have the authority to approve any other collaborator’s work. * You cannot approve your own pull requests. From b77b56a75d0583180944c7f9cf7e80db4c3f9344 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 4 Mar 2018 14:19:04 +0000 Subject: [PATCH 2/6] fixup: address comments --- COLLABORATOR_GUIDE.md | 57 ++++++++++++++++++++++--------------------- doc/onboarding.md | 11 ++++++--- 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 1fecd26e5ddf4f..0fb5e8798711f2 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -49,17 +49,18 @@ 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 -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) +requests they feel qualified to handle. Make sure this is done while being +mindful of these guidelines, the opinions of other Collaborators, and guidance +of 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 Courtesy should always be shown to individuals submitting issues and pull requests to the Node.js project. Be welcoming to first-time contributors, -identified by the GitHub ![First-time contributor](./doc/first_timer_badge.png) badge. +identified by the GitHub ![First-time contributor](./doc/first_timer_badge.png) +badge. For first-time contributors, check if the commit author is the same as the pull request author, and ask if they have configured their git @@ -80,21 +81,21 @@ necessary. ### Ready to land pull requests A pull request that is still awaiting the minimum review time is considered -`ready` as soon as the CI got kicked off, it got at least one approval and +`ready` as soon as the CI has been kicked off, it has at least one approval, and 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. +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 +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). 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 +Landing your own pull requests distributes the work load for each Collaborator 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. @@ -120,21 +121,21 @@ review by @-mention. 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, +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 -rebase. +new CI in case the PR creator pushed new code since the last CI run (due to +e.g., an addressed review comment or a 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. +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 (only) 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 +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. @@ -155,7 +156,7 @@ 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 +Collaborators, `semver-major` pull requests may be put on the TSC meeting agenda. #### Helpful resources @@ -484,9 +485,9 @@ The TSC should serve as the final arbiter where required. 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 + in case they are not. If no 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 + result. If no CI is required, please leave a comment in case none is already present. 1. Review the commit message to ensure that it adheres to the guidelines outlined in the [contributing][] guide. @@ -701,9 +702,9 @@ 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: +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 @@ -760,7 +761,7 @@ and impact of the changes on the code, the risk to ecosystem stability incurred by accepting the change, and the expected benefit that landing the commit will have for the ecosystem. -Any collaborator who feels a semver-minor commit should be landed in an LTS +Any Collaborator who feels a semver-minor commit should be landed in an LTS branch should attach the `lts-agenda` label to the pull request. The LTS WG will discuss the issue and, if necessary, will escalate the issue up to the TSC for further discussion. @@ -777,7 +778,7 @@ pulled from the staging branch into the LTS branch only when a release is being prepared and may be pulled into the LTS branch in a different order than they were landed in staging. -Any collaborator may land commits into a staging branch, but only the release +Any Collaborator may land commits into a staging branch, but only the release team should land commits into the LTS branch while preparing a new LTS release. @@ -799,7 +800,7 @@ Several LTS related issue and PR labels have been provided: * `land-on-v4.x` - tells the release team that the commit should be landed in a future v4.x release -Any collaborator can attach these labels to any PR/issue. As commits are +Any Collaborator can attach these labels to any PR/issue. As commits are landed into the staging branches, the `lts-watch-` label will be removed. Likewise, as commits are landed in a LTS release, the `land-on-` label will be removed. diff --git a/doc/onboarding.md b/doc/onboarding.md index d691c044b37a30..2ed97a97d5c698 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -86,10 +86,13 @@ onboarding session. `semver-major` label * When adding a `semver-*` label, add a comment explaining why you're adding it. Do it right away so you don't forget! - * Please add the `ready` label for PRs where the CI was just kicked off and - that are good to go (no outstanding review items), pending the CI outcome - and possibly the 48/72 hour waiting rule and the necessary LGs for - semver-major. + * Please add the `ready` label for PRs where: + * the CI has been started, + * that have no outstanding review comments and + * at least one approval, + * pending the CI outcome, + * the 48/72 hour waiting rule and + * the necessary LGs for semver-major. * [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) * This will come more naturally over time From 0652f24d179f93454fe8af7d2354c51b1dd8c36a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 4 Mar 2018 14:19:49 +0000 Subject: [PATCH 3/6] fixup --- COLLABORATOR_GUIDE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 0fb5e8798711f2..28908eaa038312 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -81,8 +81,8 @@ necessary. ### Ready to land pull requests A pull request that is still awaiting the minimum review time is considered -`ready` as soon as the CI has been kicked off, it has at least one approval, and -it has no outstanding review comments. Please always make sure to add the +`ready` as soon as the CI has been started, it has at least one approval, and 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. From 12ef5df35cfa8bf5921bc2c0369d347fa64c9f0e Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 7 Mar 2018 00:44:33 +0100 Subject: [PATCH 4/6] fixup address comments --- COLLABORATOR_GUIDE.md | 24 +++++++++++++----------- doc/onboarding.md | 2 +- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 28908eaa038312..63aec9afb90df0 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -6,7 +6,7 @@ - [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) + - [Author ready pull requests](#author-ready-pull-requests) - [Handling own pull requests](#handling-own-pull-requests) * [Accepting Modifications](#accepting-modifications) - [Code Reviews](#code-reviews) @@ -78,13 +78,13 @@ 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 +### Author ready pull requests A pull request that is still awaiting the minimum review time is considered -`ready` as soon as the CI has been started, it has at least one approval, and 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. +`author-ready` as soon as the CI has been started, it has at least one approval, +and it has no outstanding review comments. Please always make sure to add the +appropriate `author-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 @@ -97,15 +97,17 @@ rebasing). As soon as the PR is ready to land, please go ahead and do so on your own. Landing your own pull requests distributes the work load for each Collaborator 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. +[minimum time to land](#waiting-for-approvals), please add the `author-ready` +label to it so it is obvious that the PR can land as soon as the time ends. ## 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. +landed into the codebase. There may be exception to the latter (the changed code +can not be tested with a CI or similar). If that is the case, please leave a +comment that explains why the PR does not require a CI run. ### Code Reviews @@ -137,8 +139,8 @@ the CI outcome. 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. +[minimum time to land](#waiting-for-approvals), please add the `author-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 diff --git a/doc/onboarding.md b/doc/onboarding.md index 2ed97a97d5c698..f23f0ba5fa235c 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -86,7 +86,7 @@ onboarding session. `semver-major` label * When adding a `semver-*` label, add a comment explaining why you're adding it. Do it right away so you don't forget! - * Please add the `ready` label for PRs where: + * Please add the `author-ready` label for PRs where: * the CI has been started, * that have no outstanding review comments and * at least one approval, From a9e01da78fc7f748758cba032a6fe6f4a277fcf5 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 9 Mar 2018 01:46:19 +0100 Subject: [PATCH 5/6] fixup --- doc/onboarding.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index f23f0ba5fa235c..62cb6d9a3630f4 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -87,12 +87,9 @@ onboarding session. * When adding a `semver-*` label, add a comment explaining why you're adding it. Do it right away so you don't forget! * Please add the `author-ready` label for PRs where: - * the CI has been started, - * that have no outstanding review comments and - * at least one approval, - * pending the CI outcome, - * the 48/72 hour waiting rule and - * the necessary LGs for semver-major. + * the CI has been started (not necessarily finished), + * no outstanding review comments exist and + * at least one collaborator approved the PR. * [**See "Who to CC in issues"**](./onboarding-extras.md#who-to-cc-in-issues) * This will come more naturally over time From 79c48f5f3f6467b928fe8e5cacbc38c518a9eb4d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 11 Mar 2018 23:53:08 +0100 Subject: [PATCH 6/6] fixup: address comment --- COLLABORATOR_GUIDE.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 63aec9afb90df0..9f1eb177cc1386 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -88,10 +88,10 @@ 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 +If you as a Collaborator open a pull request, it is recommended to start a CI +right after (see [testing and CI](#testing-and-ci) for further information on +how to do that) and to post the link to it as well. Starting a new CI after each +update is also recommended (due to e.g., a change request in a review or due to rebasing). As soon as the PR is ready to land, please go ahead and do so on your own.