Merge pull request #3460 from ethereum/reviewAndReleaseGuidelines

Introduce Review and Release Guidelines

.github/ISSUE_TEMPLATE.md

@@ -1,10 +1,10 @@
-<!---
+<!--
 
 Steps before creating an issue: 
 
 1. I've read the documentation.
-2. I was looking for an solution on stackoverflow or something else.
-3. I was looking for an identical issue.
+2. I've tried googling or asking my question on stackoverflow.
+3. I've tried searching this repository for similar issues.
 
 -->
 
@@ -20,5 +20,8 @@ Steps before creating an issue:
 
 #### Logs
 
-#### Versions
-[NPM, Node, Web3.js, OS, device...]
+<!-- Please share any relevant logs that may help us debug your issue. -->
+
+#### Environment
+
+<!-- e.g. Versions used of: npm, Node, web3.js, OS, device -->

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,15 +1,15 @@
 ## Description
 
-Please include a summary of the change and be sure you follow the contributions rules we do provide [here](./CONTRIBUTIONS.md)
+Please include a summary of the changes and be sure to follow our [Contribution Guidelines](../CONTRIBUTIONS.md).
 
-<!--- 
+<!--
 Optional if an issue is fixed:
 Fixes #(issue)
 -->
 
 ## Type of change
 
-<!--- Please delete options that are not relevant. -->
+<!-- Please delete options that are not relevant. -->
 
 - [ ] Bug fix (non-breaking change which fixes an issue)
 - [ ] New feature (non-breaking change which adds functionality)
@@ -23,9 +23,9 @@ Fixes #(issue)
 - [ ] I have made corresponding changes to the documentation.
 - [ ] My changes generate no new warnings.
 - [ ] Any dependent changes have been merged and published in downstream modules.
-- [ ] I ran ```npm run dtslint``` with success and extended the tests and types if necessary.
-- [ ] I ran ```npm run test:unit``` with success.
-- [ ] I have executed ``npm run test:cov`` and my test cases do cover all lines and branches of the added code.
-- [ ] I ran ```npm run build-all``` and tested the resulting file/'s from  ```dist``` folder in a browser.
-- [ ] I have updated the ``CHANGELOG.md`` file in the root folder.
+- [ ] I ran `npm run dtslint` with success and extended the tests and types if necessary.
+- [ ] I ran `npm run test:unit` with success.
+- [ ] I ran `npm run test:cov` and my test cases cover all the lines and branches of the added code.
+- [ ] I ran `npm run build-all` and tested the resulting files from `dist` folder in a browser.
+- [ ] I have updated the `CHANGELOG.md` file in the root folder.
 - [ ] I have tested my code on the live network.

CHANGELOG.md

@@ -178,6 +178,8 @@ Released with 1.0.0-beta.37 code base.
 
 ### Added
 
+- Introduce review and release guidelines. (#3460)
+
 ### Changed
 
 - Change CI provider from Travis to Github Actions. (#3468)

CONTRIBUTIONS.md

@@ -6,59 +6,46 @@ that **Web3 not break**.
 
 ### Pull Requests for substantive changes (e.g. everything except comments and docs)
 
-1. Any PR that introduces a logic change should include tests. (In many cases, the tests will take
-more time to write than the actual code).
-
-2. All PRs should sit for 72 hours with the `pleasereview` tag in order to garner feedback.
-
-3. No PR should be merged until it has been reviewed, passes CI, and all reviews' comments are
-addressed.
-
-4. PRs should:
-  + have a narrow, well-defined focus.
-  + make the smallest set of changes possible to achieve their goal.
-  + include a clear description in the opening comment.
-  + preserve the conventions and stylistic consistency of any files they modify.
-
-5. Given the choice between a conservative change that mostly works and an adventurous change which
-seems better but introduces uncertainty - prefer the conservative change.
+1.  Any PR that introduces a logic change should include tests. (In many cases, the tests will take more time to write than the actual code).
+1.  All PRs should sit for 72 hours with the `pleasereview` tag in order to garner feedback.
+1.  No PR should be merged until it has been reviewed, passes CI, and all reviews' comments are
+    addressed.
+1.  PRs should:
+    1.  have a narrow, well-defined focus.
+    1.  make the smallest set of changes possible to achieve their goal.
+    1.  include a clear description in the opening comment.
+    1.  preserve the conventions and stylistic consistency of any files they modify.
+1.  Given the choice between a conservative change that mostly works and an adventurous change which seems better but introduces uncertainty - prefer the conservative change.
 
 ### Reviews
 
-The end-goal of review is to suggest useful improvements to the author. Reviews should finish with
-approval unless there are issues that would result in:
-
-1. Buggy behaviour.
-
-2. Undue maintenance burden.
+The end goal of review is to suggest useful improvements to the author. Reviews should finish with approval unless there are issues that would result in:
 
-3. Pessimisation (i.e. speed reduction / meaningful build-size increases).
+1.  Buggy behaviour.
+1.  Undue maintenance burden.
+1.  Pessimisation (i.e. speed reduction / meaningful build-size increases).
+1.  Feature reduction (i.e. it removes some aspect of functionality that users rely on).
+1.  Avoidable risk (i.e it's difficult to test or hard to anticipate the implications of, without
+    being strictly necessary to fix something broken).
 
-4. Feature reduction (i.e. it removes some aspect of functionality that users rely on).
-
-5. Avoidable risk (i.e it's difficult to test or hard to anticipate the implications of, without
-being strictly necessary to fix something broken).
+Read more in [Review Guidelines](./REVIEW.md).
 
 ### Releases
 
-1. All releases should be proposed in a PR and subject to community review for a minimum of one week.
-
-2. Release review periods should be accompanied by a published `rc` version which can be used for
-sanity checks / additional testing.
-
-3. During release review, the code is frozen unless new changes are proposed, approved and merged.
-Changes should trigger a new `rc` release and set the release clock back enough that reviewers have
-the time they need to test new changes.
+1.  All releases should be proposed in a PR and subject to community review for a minimum of one week.
+1.  Release review periods should be accompanied by a published `rc` version which can be used for sanity checks / additional testing.
+1.  During release review, the code is frozen unless new changes are proposed, approved and merged.
+1.  Changes should trigger a new `rc` release and set the release clock back enough that reviewers have the time they need to test new changes.
+1.  Regular maintainers should manually test the `rc` against a Node project and the published
+    minified bundle in a browser context. An external reviewer should verify they've done the same.
+1.  A release PR must be approved at least by two known contributors of the web3.js project.
 
-4. Regular maintainers should manually test the `rc` against a Node project and the published
-minified bundle in a browser context. An external reviewer should verify they've done the same.
-
-5. A release PR must be approved at least by two known contributors of the web3.js project
+Read more in [Release Guidelines](./RELEASE.md).
 
 ### Emergencies
 
-This topic is under org-wide discussion at https://github.com/ethereum/js-organization/issues/6
+Emergency releases are allowed to shorten waiting periods depending on the severity of the issue.
+
+There is precedent set for this in the 1.2.6 release (see [#3351](https://github.com/ethereum/web3.js/pull/3351)), where the consensus view was to make the smallest change necessary to address the emergency while waiving the `rc` process (meaning many existing additions to master were excluded).
 
-(Much of the above is borrowed from Openish, Parity and Ethers contributions docs. It's meant
-to establish clear, egalitarian criteria for making changes to the code while prioritizing the
-safety of Web3's users.)
+This topic is under further org-wide discussion at [ethereum/js-organization#6](https://github.com/ethereum/js-organization/issues/6).

README.md

@@ -142,7 +142,9 @@ npm test
 
 ### Contributing
 
-The contribution guidelines are provided in [CONTRIBUTIONS](./CONTRIBUTIONS.md)
+Please follow the [Contribution Guidelines](./CONTRIBUTIONS.md) and [Review Guidelines](./REVIEW.md).
+
+This project adheres to the [Release Guidelines](./REVIEW.md).
 
 ### Community
 

RELEASE.md

@@ -0,0 +1,130 @@
+# web3.js Release Guidelines
+
+## Version Number Definition
+
+The web3.js project follows the [semver 2.0.0 specification](https://semver.org/).
+
+### Major
+
+The `major` version has to be increased as soon as a breaking change is introduced. The definition of a breaking change is anything that requires a depending project to update their code base, build pipeline or tests.
+
+### Minor
+
+The `minor` is increased as soon as new smaller features do get introduced. A `minor` release will not affect any depending project. Such a release only introduces smaller enhancements and features a project could use.
+
+### Patch
+
+A patch release only contains required bug fixes with a low risk to impact depending project.
+
+Further details about versioning can be found in the [semver 2.0.0 specification](https://semver.org/) we follow.
+
+## Release Process Rules
+
+### Major
+
+1.  The release should get released as an `RC` version on the specified track below.
+1.  The minor `RC` version stays released for a `month` for testing purposes.
+1.  During release review, the code is frozen unless new changes are proposed, approved and merged.
+    1.  Changes should trigger a new `RC` release and set the release clock back a `week` so reviewers have the time they need to test new changes.
+1.  The project lead should manually test the `RC` against a Node project and the published
+    minified bundle in a browser context. An external reviewer should verify they have done the same.
+
+### Minor
+
+The `minor` release inherits the rules from the `major` release but has a smaller testing phase of one `week` and the release clock will be set back `2 days` on a change of the published code.
+
+### Patch
+
+Since November 2019, Web3 has been following the `minor` rules for `patch` as well, to help establish a series of non-breaking releases.
+
+## Prereleases
+
+### Release Candidate
+
+A release candidate gets published on the `rc` track and is no longer under active development. A release candidate gets released with the version number defined in the [semver 2.0.0 specification](https://semver.org/) specification.
+
+### Beta Release
+
+A `beta` release gets published on the `beta` track and is under active developemnt. The version number of the `beta` release gets defined as specified in the [semver 2.0.0](https://semver.org/) document.
+
+## Long Term Support (LTS)
+
+> Note: This section has not yet been decided and so for now is purely informative.
+
+To provide safety over longer periods, it is important to have versions tagged as `LTS`.
+
+| Release |    Status     | Initial Release |   LTS Start   | End-of-Life |
+| :-----: | :-----------: | :-------------: | :-----------: | :---------: |
+|   1.x   |      LTS      |  24. Jul. 2017  | 23. Jul. 2019 |     TBD     |
+|   2.x   | Alpha Preview |  13. Jul. 2019  |      TBD      |     TBD     |
+
+## Deprecation Rules
+
+Because breaking changes are always happening during the development of a project it is required to define rules on how we inform depending projects about coming changes with enough time to react. This doesn't mean that we plan to introduce breaking changes but sometimes they are required.
+
+1.  Deprecations will be announced in our `CHANGELOG` and in the description of a release.
+2.  Deprecated functionalities should be marked in the developer documentation, function documentation, and the related types as deprecated.
+3.  A deprecated functionality stays deprecated for the next `X (TBD)` increases of the minor version.
+
+# web3.js Release Process
+
+The following describes the steps required to release a new version of `web3.js`. It is followed to adhere to community standards and expectations.
+
+## Release Candidate (RC) Release Procedure
+
+1.  Create a GitHub draft release.
+    1.  [Example](https://github.com/ethereum/web3.js/releases/tag/v1.2.7-rc.0) - should contain at a minimum: release notes, changelog, any other important notes.
+    1.  Request review on the draft release from a web3.js contributor ([@cgewecke](https://github.com/cgewecke)) for completeness, grammar, etc.
+1.  Create release branch (e.g. `release/1.2.7`).
+1.  Update and commit `CHANGELOG.md`.
+    1.  Move section header `[Unreleased]` to bottom.
+    1.  Add next anticipated release version number to bottom (as a placeholder for new changelog entries).
+1.  Create release commit and tags e.g. `lerna version 1.2.7-rc.0 --no-push`
+    1.  (updates package version numbers, builds minified file (for `1.x`), creates release commit and tags.)
+1.  For `1.x`, save [index.html](https://github.com/ethereum/web3.js/blob/1.x/scripts/html/index.html) to your local machine and test loading minified file to ensure its integrity. Be sure to delete `index.html` after.
+1.  Check git working state is clean. Sometimes files modified by gulp are not included. If so, run `git commit --amend` and `git tag -d 1.2.7-rc.0` and `git tag 1.2.7-rc.0`. 
+1.  Push release branch to origin with tags `git push origin release/1.2.7 --follow-tags`.
+1.  Create release PR as draft ([example](https://github.com/ethereum/web3.js/pull/3351)).
+1.  Ensure CI is green / passing.
+    1.  (spend time here inspecting the CI logs to ensure everything looks valid and results were reported correctly)
+1.  Run `npm run publish from-package -- --dist-tag rc`.
+    1. Lerna can sometimes have difficulty with the number of packages we have. If the above command is unsuccessful, for every unpublished package run: `lerna publish --scope="package-name"` `npm dist-tag add <package-name>@<version> rc`)
+1.  Publish the GitHub release.
+1.  A GitHub Webhook should trigger the ReadTheDocs build after the release is published.
+    1.  (The build may sometimes need to be manually triggered in ReadTheDocs admin panel. If the version does not appear, create a build of a previous version to refresh the list.)
+    1.  Activate the new version.
+1.  Request PR review from key contributors:
+        1.  Chris from EthereumJS ([@cgewecke](https://github.com/cgewecke))
+        1.  Patricio from Nomic Labs ([@alcuadrado](https://github.com/alcuadrado))
+        1.  Michael from Embark ([@michaelsbradleyjr](https://github.com/michaelsbradleyjr))
+        1.  Nicholas from Truffle ([@gnidan](https://github.com/gnidan))
+        1.  If touches or affects ENS: Nick Johnson ([@Arachnid](https://github.com/Arachnid))
+1.  Wait 1 week for community discourse and 2 reviewer approvals.
+    1.  (if release is an emergency patch, time limit may be reduced relative to severity.)
+
+## Formal Release Procedure
+
+1.  Create GitHub draft release from text of `rc` release.
+1.  Checkout release branch (e.g. `release/1.2.7`).
+1.  Create and push release commit and tags: `lerna version 1.2.7 --force-publish --no-push`
+1.  Check git working state is clean. Sometimes files modified by gulp are not included. If so, run `git commit --amend` and `git tag -d 1.2.7` and `git tag 1.2.7`. 
+1.  Push release branch to origin with tags `git push origin release/1.2.7 --follow-tags`.
+1.  Publish the GitHub release.
+1.  A GitHub Webhook should trigger the ReadTheDocs build after the release is published.
+    1.  (The build may sometimes need to be manually triggered in ReadTheDocs admin panel. If the version does not appear, create a build of a previous version to refresh the list.)
+    1.  Activate the new version.
+    1.  Set the version to default.
+1.  Run `npm run publish from-package`.
+1.  Merge release PR.
+1.  Share the release announcement on:
+    1.  (_Note:_ There is a delay on npm between different regions, so all may not see the release immediately.)
+    1.  Twitter
+    1.  Gitter
+    1.  Ethereum JavaScript Community (EJC) Discord
+    1.  Reddit
+    1.  Depending on release's significance to certain projects, write to:
+        1.  Fabio from 0x ([@fabioberger](https://github.com/fabioberger))
+        1.  Santiago from OpenZeppelin ([@spalladino](https://github.com/spalladino))
+        1.  Pedro from WalletConnect ([@pedrouid](https://github.com/pedrouid))
+        1.  Josh from FunFair ([@joshstevens19](https://github.com/joshstevens19))
+        1.  Truffle, Gnosis, Aragon, Embark, Alchemy, Buidler, Remix

REVIEW.md

@@ -0,0 +1,27 @@
+# Review Guidelines
+
+Our review guidelines are intended to provide clear steps for PR proposers and reviewers.
+
+Only published PRs will be considered for review. Draft PRs will be considered in-progress and not yet ready for review.
+
+## Rules
+
+*   [ ] PR follows the provided [template](.github/PULL_REQUEST_TEMPLATE.md).
+*   [ ] PR doesn't contain unneccessary changes.
+*   [ ] The changed code preserves the conventions and stylistic consistency of the project.
+*   [ ] PR uses labels accordingly. (new labels may be suggested)
+*   [ ] PR includes unit and e2e tests if related to any logic changes.
+    *   [ ] The code coverage rate did not decrease.
+    *   [ ] The error case is always tested.
+    *   [ ] The description of the test case is self-describing.
+    *   [ ] Test cases are grouped for clarity.
+*   [ ] A PR may only be merged if the following conditions are fulfilled:
+    *   [ ] The correct base branch is selected.
+    *   [ ] Any new files contain the web3.js file header.
+    *   [ ] The documentation was updated (if applicable).
+    *   [ ] The CHANGELOG was updated accordingly.
+    *   [ ] The CI with QA passes succesfully.
+        *   [ ] The CI logs were manually checked to ensure false positives were not reported.
+    *   [ ] All comments have been addressed.
+    *   [ ] Doesn't add undue maintenance burden.
+    *   [ ] Doesn't increase the bundle size or is clearly explained why.