From 8763d99cca3e20cae7bf8943e4d7c66bf6e0e8c6 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Thu, 20 Apr 2023 19:14:06 +0200 Subject: [PATCH] docs: update contributing guidelines (#859) * update PR templates * update contributing guidelines * apply review suggestions * Update CONTRIBUTING.md Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> * apply review suggestions * apply review suggestions * Update CONTRIBUTING.md Co-authored-by: MSalopek <35486649+MSalopek@users.noreply.github.com> * apply review suggestions --------- Co-authored-by: Shawn <44221603+smarshall-spitzbart@users.noreply.github.com> Co-authored-by: MSalopek <35486649+MSalopek@users.noreply.github.com> --- .github/PULL_REQUEST_TEMPLATE.md | 68 +++++---- .github/PULL_REQUEST_TEMPLATE/docs.md | 37 +++++ .github/PULL_REQUEST_TEMPLATE/other.md | 32 +++++ CONTRIBUTING.md | 192 +++++++++++++++++++++---- 4 files changed, 271 insertions(+), 58 deletions(-) create mode 100644 .github/PULL_REQUEST_TEMPLATE/docs.md create mode 100644 .github/PULL_REQUEST_TEMPLATE/other.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index a631a95f0a..936fe1c59f 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,43 +1,49 @@ -# Description + -Please include a summary of the changes and the related issue. If the issue was ambiguous try to clarify it in this section. +## Description -## Linked issues +Closes: #XXXX -Closes: `#` + -## Type of change +--- -If you've checked more than one of the first three boxes, consider splitting this PR into multiple PRs! +### Author Checklist -- [ ] `Feature`: Changes and/or adds code behavior, irrelevant to bug fixes -- [ ] `Fix`: Changes and/or adds code behavior, specifically to fix a bug -- [ ] `Refactor`: Changes existing code style, naming, structure, etc. -- [ ] `Testing`: Adds testing -- [ ] `Docs`: Adds documentation +*All items are required. Please add a note to the item if the item is not applicable and +please add links to any relevant follow up issues.* -## Regression tests +I have... -If `Refactor`, describe the new or existing tests that verify no behavior was changed or added where refactors were introduced. +* [ ] Included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title +* [ ] Added `!` to the type prefix if API or client breaking change +* [ ] Targeted the correct branch (see [PR Targeting](https://github.com/cosmos/interchain-security/blob/main/CONTRIBUTING.md#pr-targeting)) +* [ ] Provided a link to the relevant issue or specification +* [ ] Followed the guidelines for [building SDK modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules) +* [ ] Included the necessary unit and integration [tests](https://github.com/cosmos/interchain-security/blob/main/CONTRIBUTING.md#testing) +* [ ] Added a changelog entry to `CHANGELOG.md` +* [ ] Included comments for [documenting Go code](https://blog.golang.org/godoc) +* [ ] Updated the relevant documentation or specification +* [ ] Reviewed "Files changed" and left comments if necessary +* [ ] Confirmed all CI checks have passed -## New behavior tests +### Reviewers Checklist -If `Feature` or `Fix`, describe the new or existing tests that verify the new behavior is correct and expected. +*All items are required. Please add a note if the item is not applicable and please add +your handle next to the items reviewed if you only reviewed selected items.* -## Versioning Implications +I have... -- [ ] This PR will affect [semantic versioning as defined for ICS](../CONTRIBUTING.md#semantic-versioning) - -If the above box is checked, which version should be bumped? - -- [ ] `MAJOR`: Consensus breaking changes to both the provider and consumers(s), including updates/breaking changes to IBC communication between provider and consumer(s) -- [ ] `MINOR`: Consensus breaking changes which affect either only the provider or only the consumer(s) -- [ ] `PATCH`: Non consensus breaking changes - -## Targeting - -Please select one of the following: - -- [ ] This PR is only relevant to main -- [ ] This PR is relevant to main, and should also be back-ported to ____ (ex: v1.0.0 and v1.1.0) -- [ ] This PR is only relevant to ____ (ex: v1.0.0, v1.1.0, and v1.2.0) +* [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title +* [ ] confirmed `!` in the type prefix if API or client breaking change +* [ ] confirmed all author checklist items have been addressed +* [ ] reviewed state machine logic +* [ ] reviewed API design and naming +* [ ] reviewed documentation is accurate +* [ ] reviewed tests and test coverage diff --git a/.github/PULL_REQUEST_TEMPLATE/docs.md b/.github/PULL_REQUEST_TEMPLATE/docs.md new file mode 100644 index 0000000000..59ccc2000d --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/docs.md @@ -0,0 +1,37 @@ +## Description + +Closes: #XXXX + + + +--- + +### Author Checklist + +*All items are required. Please add a note to the item if the item is not applicable and +please add links to any relevant follow up issues.* + +I have... + +- [ ] included the correct `docs:` prefix in the PR title +- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/interchain-security/blob/main/CONTRIBUTING.md#pr-targeting)) +- [ ] provided a link to the relevant issue or specification +- [ ] reviewed "Files changed" and left comments if necessary +- [ ] confirmed all CI checks have passed + +### Reviewers Checklist + +*All items are required. Please add a note if the item is not applicable and please add +your handle next to the items reviewed if you only reviewed selected items.* + +I have... + +- [ ] Confirmed the correct `docs:` prefix in the PR title +- [ ] Confirmed all author checklist items have been addressed +- [ ] Confirmed that this PR only changes documentation +- [ ] Reviewed content for consistency +- [ ] Reviewed content for spelling and grammar +- [ ] Tested instructions (if applicable) +- [ ] Checked that the documentation website can be built and deployed successfully (run `make build-docs`) + diff --git a/.github/PULL_REQUEST_TEMPLATE/other.md b/.github/PULL_REQUEST_TEMPLATE/other.md new file mode 100644 index 0000000000..a2c13d938d --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE/other.md @@ -0,0 +1,32 @@ +## Description + +Closes: #XXXX + + + +--- + +### Author Checklist + +*All items are required. Please add a note to the item if the item is not applicable and +please add links to any relevant follow up issues.* + +I have... + +- [ ] Included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title +- [ ] Targeted the correct branch (see [PR Targeting](https://github.com/cosmos/interchain-security/blob/main/CONTRIBUTING.md#pr-targeting)) +- [ ] Provided a link to the relevant issue or specification +- [ ] Reviewed "Files changed" and left comments if necessary +- [ ] Confirmed all CI checks have passed + +### Reviewers Checklist + +*All items are required. Please add a note if the item is not applicable and please add +your handle next to the items reviewed if you only reviewed selected items.* + +I have... + +- [ ] Confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title +- [ ] Confirmed all author checklist items have been addressed +- [ ] Confirmed that this PR does not change production code diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 83d171f441..e148b198fa 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,49 +1,178 @@ # Contributing - [Contributing](#contributing) + - [Team Organization](#team-organization) - [Architecture Decision Records (ADR)](#architecture-decision-records-adr) - - [PR Targeting](#pr-targeting) + - [ADR Proposals](#adr-proposals) - [Development Procedure](#development-procedure) - - [Semantic Versioning](#semantic-versioning) - - [Testing](#testing) + - [Testing](#testing) + - [Pull Requests](#pull-requests) + - [Pull Request Templates](#pull-request-templates) + - [Requesting Reviews](#requesting-reviews) + - [Updating Documentation](#updating-documentation) + - [Dependencies](#dependencies) - [Protobuf](#protobuf) - -Thank you for considering making contributions to Interchain Security (ICS)! - -To contribute, you could start by browsing [new issues](https://github.com/cosmos/interchain-security/issues). + - [Branching Model and Release](#branching-model-and-release) + - [Semantic Versioning](#semantic-versioning) + - [Backwards Compatibility](#backwards-compatibility) + - [PR Targeting](#pr-targeting) + +Thank you for considering making contributions to the Interchain Security (ICS) repository! 🎉👍 + +Contributing to this repo can mean many things such as participating in +discussion or proposing code changes. To ensure a smooth workflow for all +contributors, a general procedure for contributing has been established: + +1. Start by browsing [existing issues](https://github.com/cosmos/interchain-security/issues) and [discussions](https://github.com/cosmos/interchain-security/discussions). If you are looking for something interesting or if you have something in your mind, there is a chance it had been discussed. + * Looking for a good place to start contributing? How about checking out some [good first issues](https://github.com/cosmos/interchain-security/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22) or [bugs](https://github.com/cosmos/interchain-security/issues?q=is%3Aopen+is%3Aissue+label%3Abug)? +2. Determine whether a GitHub issue or discussion is more appropriate for your needs: + 1. If you want to propose something new that requires specification or an additional design, or you would like to change a process, start with a [new discussion](https://github.com/cosmos/interchain-security/discussions/new/choose). With discussions, we can better handle the design process using discussion threads. A discussion usually leads to one or more issues. + 2. If the issue you want addressed is a specific proposal or a bug, then open a [new issue](https://github.com/cosmos/interchain-security/issues/new/choose). + 3. Review existing [issues](https://github.com/cosmos/interchain-security/issues) to find an issue you'd like to help with. +3. Participate in thoughtful discussion on that issue. +4. If you would like to contribute: + 1. Ensure that the proposal has been accepted. + 2. Ensure that nobody else has already begun working on this issue. If they have, + make sure to contact them to collaborate. + 3. If nobody has been assigned for the issue and you would like to work on it, + make a comment on the issue to inform the community of your intentions + to begin work. +5. To submit your work as a contribution to the repository, follow standard GitHub best practices. See [pull request guideline](#pull-requests) below. + +**Note:** For very small or trivial issues such as typos, you are not required to open an issue before submitting a PR. +For complex problems or features, please make sure to open an issue and provide context and problem description. +PRs opened before adequate design discussion has taken place in a GitHub issue have a high likelihood of being rejected without review. + +## Team Organization + +ICS has many stakeholders contributing and shaping the project. The Core ICS team is composed of Informal Systems developers. Any long-term contributors and additional maintainers from other projects are welcome. We use self-organizing principles to coordinate and collaborate across organizations in structured "EPIC" that focus on specific problem domains or architectural components of ICS. For details, see the [GitHub Project board](https://github.com/orgs/cosmos/projects/28/views/11). + +The developers work in sprints, which are available in a [GitHub Project](https://github.com/orgs/cosmos/projects/28/views/2). ## Architecture Decision Records (ADR) -When proposing an architecture decision for ICS, please start by opening an [issue](https://github.com/cosmos/interchain-security/issues/new/choose) with a summary of the proposal. Once the proposal has been discussed and there is rough alignment on a high-level approach to the design, you may either start development, or write an ADR. +When proposing an architecture decision for ICS, please start by opening an [issue](https://github.com/cosmos/interchain-security/issues/new/choose) or a [discussion](https://github.com/cosmos/interchain-security/discussions/new) with a summary of the proposal. Once the proposal has been discussed and there is rough alignment on a high-level approach to the design, you may either start development, or write an ADR. If your architecture decision is a simple change, you may contribute directly without writing an ADR. However, if you are proposing a significant change, please include a corresponding ADR. In certain circumstances, the architecture decision may require changes to the ICS spec. Note that the spec is responsible for defining language-agnostic, implementation-agnostic behaviors for the ICS protocol. Whereas ADRs are responsible for communicating implementation decisions contained within this repo. -To create an ADR, follow the [template](https://github.com/cosmos/interchain-security/blob/main/docs/architecture/adr-template.md) and [doc](https://github.com/cosmos/interchain-security/blob/main/docs/architecture/README.md). If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/interchain-security/tree/main/docs/architecture). +To create an ADR, follow the [template](./docs/docs/adrs/adr-template.md) and [doc](./docs/docs/adrs/Readme.md). If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/interchain-security/tree/main/docs/architecture). ### ADR Proposals Architecture Decision Records (ADRs) may be proposed by any contributors or maintainers of ICS. ADRs are intended to be iterative, and may be merged into `main` while still in a `Proposed` status -## PR Targeting +## Development Procedure + +* The latest state of development is on `main`. +* `main` must never fail `make lint` or `make test`. +* Create a branch to start work: + * Fork the repo (core developers must create a branch directly in the ICS repo), + branch from the HEAD of `main`, make some commits, and submit a PR to `main`. + * For core developers working within the `interchain-security` repo, follow branch name conventions to ensure clear + ownership of branches: `{moniker}/{issue#}-branch-name`. + * See [Branching Model](#branching-model-and-release) for more details. +* Be sure to run `make format` before every commit. The easiest way + to do this is have your editor run it for you upon saving a file (most of the editors + will do it anyway using a pre-configured setup of the programming language mode). -ICS adheres to the [trunk based development branching model](https://trunkbaseddevelopment.com/). +Code is merged into main through pull request procedure. -All feature additions and all bug fixes must be targeted against `main`. Exception is for bug fixes which may only be relevant to a previously released version. In that case, the bug fix PR must target against the release branch. +### Testing -If needed, we will backport a commit from `main` to a release branch with appropriate consideration of versioning. +Appropriate tests should be written with a new feature, and all existing tests should pass. See [docs/testing.md](./docs/old/testing.md) for more information. -## Development Procedure +### Pull Requests + +Before submitting a pull request: + +* synchronize your branch with the latest main and resolve any arising conflicts, i.e., + - either `git fetch origin/main && git merge origin/main` + - or `git fetch origin/main && git rebase -i origin/main` +* run `make lint` and `make test` to ensure that all checks and tests pass. + +Then: + +1. If you have something to show, **start with a `Draft` PR**. It's good to have early validation of your work and we highly recommend this practice. A Draft PR also indicates to the community that the work is in progress. + Draft PRs also help the core team provide early feedback and ensure the work is in the right direction. +2. When the code is complete, change your PR from `Draft` to `Ready for Review`. +3. Go through the actions for each checkbox present in the PR template description. The PR actions are automatically provided for each new PR. +4. Be sure to include a relevant changelog entry in the `Unreleased` section of `CHANGELOG.md` (see file for log format). The entry should be on top of all others changes in the section. + +PRs must have a category prefix that is based on the type of changes being made (for example, `fix`, `feat`, +`refactor`, `docs`, and so on). The *type* must be included in the PR title as a prefix (for example, +`fix: `). This convention ensures that all changes that are committed to the base branch follow the +[Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification. +Additionally, **each PR should only address a single issue**. + +NOTE: when merging, GitHub will squash commits and rebase on top of the main. + +### Pull Request Templates + +There are three PR templates. The [default template](./.github/PULL_REQUEST_TEMPLATE.md) is for types `fix`, `feat`, and `refactor`. We also have a [docs template](./.github/PULL_REQUEST_TEMPLATE/docs.md) for documentation changes and an [other template](./.github/PULL_REQUEST_TEMPLATE/other.md) for changes that do not affect production code. When previewing a PR before it has been opened, you can change the template by adding one of the following parameters to the url: + +* `template=docs.md` +* `template=other.md` + +### Requesting Reviews + +In order to accommodate the review process, the author of the PR must complete the author checklist +(from the pull request template) +to the best of their abilities before marking the PR as "Ready for Review". If you would like to +receive early feedback on the PR, open the PR as a "Draft" and leave a comment in the PR indicating +that you would like early feedback and tagging whoever you would like to receive feedback from. + +Codeowners are marked automatically as the reviewers. + +All PRs require at least two review approvals before they can be merged (one review might be acceptable in +the case of minor changes to [docs](./.github/PULL_REQUEST_TEMPLATE/docs.md) or [other](./.github/PULL_REQUEST_TEMPLATE/other.md) changes that do not affect production code). Each PR template has a reviewers checklist that must be completed before the PR can be merged. Each reviewer is responsible +for all checked items unless they have indicated otherwise by leaving their handle next to specific +items. In addition, use the following review explanations: + +* `LGTM` without an explicit approval means that the changes look good, but you haven't thoroughly reviewed the reviewer checklist items. +* `Approval` means that you have completed some or all of the reviewer checklist items. If you only reviewed selected items, you must add your handle next to the items that you have reviewed. In addition, follow these guidelines: + * You must also think through anything which ought to be included but is not + * You must think through whether any added code could be partially combined (DRYed) with existing code + * You must think through any potential security issues or incentive-compatibility flaws introduced by the changes + * Naming must be consistent with conventions and the rest of the codebase + * Code must live in a reasonable location, considering dependency structures (for example, not importing testing modules in production code, or including example code modules in production code). + * If you approve the PR, you are responsible for any issues mentioned here and any issues that should have been addressed after thoroughly reviewing the reviewer checklist items in the pull request template. +* If you sat down with the PR submitter and did a pairing review, add this information in the `Approval` or your PR comments. +* If you are only making "surface level" reviews, submit notes as a `comment` review. + +### Updating Documentation + +If you open a PR in ICS, it is mandatory to update the relevant documentation in `/docs`. + +## Dependencies + +We use [Go Modules](https://github.com/golang/go/wiki/Modules) to manage +dependency versions. -- The latest state of development is on `main` -- `main` must never fail `make test` -- Create a branch or fork the repo to start work -- Once development is completed, or during development as a draft, you may open a pull request. The pull request description should follow the [default template](./.github/PULL_REQUEST_TEMPLATE.md). Note that a PR can be of one or multiple types, including `Feature`, `Fix`, `Refactor`, `Testing`, and `Docs`. Please try to minimize the number of categories that you could classify a single PR as. -- It's notable that your PR may have versioning implications. See [Semantic Versioning](#semantic-versioning) for more information -- Code is squash merged through the PR approval process +The main branch of every Cosmos repository should just build with `go get`, +which means they should be kept up-to-date with their dependencies so we can +get away with telling people they can just `go get` our software. -## Semantic Versioning +When dependencies in ICS's `go.mod` are changed, it is generally accepted practice +to delete `go.sum` and then run `go mod tidy`. + +Since some dependencies are not under our control, a third party may break our +build, in which case we can fall back on `go mod tidy -v`. + +## Protobuf + +We use [Protocol Buffers](https://developers.google.com/protocol-buffers) along with [buf](https://buf.build/docs/) to generate code for use in ICS. + +For deterministic behavior around Protobuf tooling, everything is containerized using Docker. Make sure to have Docker installed on your machine, or head to [Docker's website](https://docs.docker.com/get-docker/) to install it. + +To generate the protobuf stubs, you can run `make proto-gen`. + +## Branching Model and Release + +ICS adheres to the [trunk based development branching model](https://trunkbaseddevelopment.com/). User branches should start with a user name, example: `{moniker}/{issue#}-branch-name`. + +### Semantic Versioning ICS uses a variation of [semantic versioning](https://semver.org/). @@ -61,14 +190,23 @@ Pure documentation, testing, and refactoring PRs do not require a version bump. A MAJOR version of ICS will always be backwards compatible with the previous MAJOR version of ICS. Versions before that are not supported. For example, a provider chain could run ICS at version 3.4.5, and would be compatible with consumers running ICS at 2.0.0, 2.1.2, 3.2.1, but not 1.2.7. -## Testing +### PR Targeting + +Ensure that you base and target your PR on the `main` branch. + +All feature additions and all bug fixes must be targeted against `main`. Exception is for bug fixes which are only related to a released version. In that case, the related bug fix PRs must target against the release branch. + +If needed, we will backport a commit from `main` to a release branch with appropriate consideration of versioning. + + + + + + + + -Appropriate tests should be written with a new feature, and all existing tests should pass. See [docs/testing.md](./docs/testing.md) for more information. -## Protobuf -We use [Protocol Buffers](https://developers.google.com/protocol-buffers) along with [gogoproto](https://github.com/gogo/protobuf) to generate code for use in ICS. -For determinstic behavior around Protobuf tooling, everything is containerized using Docker. Make sure to have Docker installed on your machine, or head to [Docker's website](https://docs.docker.com/get-docker/) to install it. -To generate the protobuf stubs, you can run `make proto-gen`.