From a39869aa3f7c7aa13811a1edb1917591be3143df Mon Sep 17 00:00:00 2001 From: Marko Date: Tue, 28 May 2024 10:42:20 +0200 Subject: [PATCH] chore: update contributing (#20461) --- .github/PULL_REQUEST_TEMPLATE.md | 15 +++- .github/PULL_REQUEST_TEMPLATE/docs.md | 38 --------- CONTRIBUTING.md | 112 ++++++++++++++++---------- 3 files changed, 85 insertions(+), 80 deletions(-) delete mode 100644 .github/PULL_REQUEST_TEMPLATE/docs.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index b1a5d17daa..5538d72b38 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -14,7 +14,18 @@ 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 +* [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title, you can find examples of the prefixes below: + * `feat`: A new feature + * `fix`: A bug fix + * `docs`: Documentation only changes + * `style`: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc) + * `refactor`: A code change that neither fixes a bug nor adds a feature + * `perf`: A code change that improves performance + * `test`: Adding missing tests or correcting existing tests + * `build`: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm) + * `ci`: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs) + * `chore`: Other changes that don't modify src or test files + * `revert`: Reverts a previous commit * [ ] confirmed `!` in the type prefix if API or client breaking change * [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) * [ ] provided a link to the relevant issue or specification @@ -29,6 +40,8 @@ I have... *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.* +Please see [Pull Request Reviewer section in the contributing guide](../CONTRIBUTING.md#reviewer) for more information on how to review a pull request. + I have... * [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title diff --git a/.github/PULL_REQUEST_TEMPLATE/docs.md b/.github/PULL_REQUEST_TEMPLATE/docs.md deleted file mode 100644 index 5f3dadd700..0000000000 --- a/.github/PULL_REQUEST_TEMPLATE/docs.md +++ /dev/null @@ -1,38 +0,0 @@ -# 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/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) -* [ ] provided a link to the relevant issue or specification -* [ ] followed the [documentation writing guidelines](https://github.com/cosmos/cosmos-sdk/blob/main/docs/DOC_WRITING_GUIDELINES.md) -* [ ] 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 thoroughness -* [ ] reviewed content for spelling and grammar -* [ ] tested instructions (if applicable) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 22f82c6150..1a5fe96f23 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,6 +6,10 @@ * [Testing](#testing) * [Pull Requests](#pull-requests) * [Pull Request Templates](#pull-request-templates) + * [Pull Request Accountability](#pull-request-accountability) + * [Owner](#owner) + * [Reviewer](#reviewer) + * [External Contributors](#external-contributors) * [Requesting Reviews](#requesting-reviews) * [Updating Documentation](#updating-documentation) * [RFC & ADR](#rfc--adr) @@ -20,9 +24,6 @@ * [Strategy Discovery](#strategy-discovery) * [Concept Approval](#concept-approval) * [Time Bound Period](#time-bound-period) - * [Approval Committee & Decision Making](#approval-committee--decision-making) - * [Committee Members](#committee-members) - * [Committee Criteria](#committee-criteria) * [Implementation & Release Approval](#implementation--release-approval) Thank you for considering making contributions to the Cosmos SDK and related repositories! @@ -58,7 +59,7 @@ spell checks all files and will automatically open PRs for any spelling errors. ## Teams Dev Calls -The Cosmos SDK has many stakeholders contributing and shaping the project. The Core SDK team is composed of Interchain GmbH and Regen Network Development 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 the Cosmos SDK. +The Cosmos SDK has many stakeholders contributing and shaping the project. The Core SDK team is composed of Binary Builders & Zondax. 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 the Cosmos SDK. The developers work in sprints, which are available in a [GitHub Project](https://github.com/orgs/cosmos/projects/26/views/22). The current EPICs are pinned at the top of the [issues list](https://github.com/cosmos/cosmos-sdk/issues). @@ -66,14 +67,14 @@ The important development announcements are shared on [Discord](https://discord. To synchronize we have few major meetings: -* Cosmos SDK Sprint Review on Monday and Thursday at 14:00 UTC (limited participation to core devs). -* Cosmos SDK Community Call on Thursday at 16:00 UTC. +* Cosmos SDK Standup on Monday and Thursday at 14:00 UTC (limited participation to core devs). +* Cosmos SDK Community Call on the second Thursday of the month at 13:00 UTC. -If you would like to join one of the community call, then please contact us on [Discord](https://discord.gg/interchain) or reach out directly to Marko (@tac0turtle). +If you would like to join one of the community call, then please request to join the [Cosmos SDK Google Group](https://groups.google.com/g/cosmos-sdk-community). ## Architecture Decision Records (ADR) -When proposing an architecture decision for the Cosmos SDK, please start by opening an [issue](https://github.com/cosmos/cosmos-sdk/issues/new/choose) or a [discussion](https://github.com/cosmos/cosmos-sdk/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, the [ADR creation process](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/PROCESS.md) can begin. We are following this process to ensure all involved parties are in agreement before any party begins coding the proposed implementation. If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/cosmos-sdk/tree/main/docs/architecture). +When proposing an architecture decision for the Cosmos SDK, please start by opening an [issue](https://github.com/cosmos/cosmos-sdk/issues/new/choose). Once the proposal has been discussed and there is rough alignment on a high-level approach to the design, the [ADR creation process](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/PROCESS.md) can begin. We are following this process to ensure all involved parties are in agreement before any party begins coding the proposed implementation. If you would like to see examples of how these are written, please refer to the current [ADRs](https://github.com/cosmos/cosmos-sdk/tree/main/docs/architecture). ## Development Procedure @@ -121,7 +122,7 @@ PRs must have a category prefix that is based on the type of changes being made [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification. Additionally, each PR should only address a single issue. -Pull requests are merged automatically using [`A:automerge` action](https://mergify.io/features/auto-merge). +Pull requests are merged automatically using the automerge functionality of Github. NOTE: when merging, GitHub will squash commits and rebase on top of the main. @@ -131,6 +132,65 @@ There are three PR templates. The [default template](./.github/PULL_REQUEST_TEMP * `template=docs.md` +### Pull Request Accountability + +#### Owner + +The pull request owner is responsible for ensuring that the PR is ready for review and merging after reviews are delivered. This includes: + +* Ensuring that the PR is up to date with the latest changes in the main branch. +* Ensuring that the PR passes all checks. +* Ensuring that the PR has a clear description of the changes. +* Ensuring that the PR has a clear description of the testing strategy. +* Ensuring that the PR has a clear description of the impact of the changes. +* Ensuring that the PR has a clear description of the risks associated with the changes. +* Ensuring that the PR has a clear description of the next steps. +* Ensuring that the PR has a clear description of the dependencies. + +The pull request owner is responsible for assigning reviewers on the team, responding to feedback, and ensuring that the PR is merged in a timely manner. If a PR is reviewed, but an approval is not given by the reviewer the pull request owner is responsible for addressing the feedback, ensuring that the PR is ready for review again and notifying the reviewers that the PR is ready for review. + +Once approvals have been given by the reviewer(s) it is the responsibility of the pull request owner to merge the PR. + +#### Reviewer + +Reviewers or other contributors should not merge main into the PR unless discussed with the pull request owner and ownership has been transferred. + +The reviewer is responsible for ensuring that the PR meets the following criteria: + +##### Code Quality + +* *Readability*: Ensure the code is easy to read and understand. Check for clear and concise variable names, appropriate comments, and overall readability. +* *Coding Standards*: Verify adherence to the team’s coding standards and style guides. This includes indentation, spacing, naming conventions, and file organization. +* *Code Structure*: Check for proper use of functions, classes, and modules. Ensure the code is organized logically and is modular. +* *Complexity*: Look for complex code that could be simplified. Ensure there are no nested loops. + +##### Functionality + +* *Correctness*: Verify that the code performs the intended function correctly. Check the logic and ensure that edge cases are handled. +* *Bug Fixes*: Ensure any reported bugs are adequately addressed. Verify that the fixes resolve the issues without introducing new bugs. +* *Feature Implementation*: Confirm that new features are implemented as specified in the requirements or user stories. + +##### Testing + +* *Test Coverage*: Check that there are sufficient unit tests, integration tests and E2E tests for the new code. Ensure tests cover both normal and edge cases. +* *Test Quality*: Review the quality of the tests. Ensure they are meaningful and not just checking trivial cases. +* *Passing Tests*: Verify that all tests pass, including any new tests added with the PR. + +##### Documentation + +* *Code* Comments: Ensure there are comments explaining non-obvious parts of the code. +* *API Documentation*: Verify that any new or modified public methods, classes, or modules are properly documented. +* *User Documentation*: Check for updates to user documentation, if the PR includes changes that affect the user experience. + +##### Performance + +* *Efficiency*: Ensure the code performs efficiently and does not introduce performance bottlenecks. +* *Resource Usage*: Check for appropriate use of resources, such as memory and CPU. Ensure there are no memory leaks or excessive resource consumption. + +### External Contributors + +External contributors can not manage getting reviewers and assigning reviewers to PRs. When an external contribution requires reviewers, they will be assigned in the team meeting or adhoc based on current workloads. One of the reviewers will be assigned the owner of the PR and will be responsible for ensuring the PR is ready for review and merging after reviews are delivered. The owner has the right to overtake the contribution if the external contributor is not responsive or the PR is not moving forward. + ### Requesting Reviews In order to accommodate the review process, the author of the PR must complete the author checklist @@ -157,6 +217,7 @@ items. In addition, use the following review explanations: * 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 on the Cosmos SDK, it is mandatory to update the relevant documentation in `/docs`. @@ -319,43 +380,12 @@ If an individual Pull Request for an ADR needs more time than 2 weeks to reach r in current state (`Draft` or `Proposed`), with its contents updated to summarize the current state of its discussion. -If an ADR is taking longer than 4 weeks to reach a final conclusion, the **Concept Approval Committee** -should convene to rectify the situation by either: +If an ADR is taking longer than 4 weeks to reach a final conclusion, there should be a synchronous meeting with reviewers and all stake holders * unanimously setting a new time bound period for this ADR * making changes to the Concept Approval Process (as outlined here) * making changes to the members of the Concept Approval Committee -#### Approval Committee & Decision Making - -In absence of general consensus, decision making requires 1/2 vote from the two members -of the **Concept Approval Committee**. - -#### Committee Members - -* Core Members: **Aaron** (Regen), **Bez** (IG) - -#### Committee Criteria - -Members must: - -* Participate in all or almost all ADR discussions, both on GitHub as well as in bi-weekly Architecture Review - meetings -* Be active contributors to the Cosmos SDK, and furthermore should be continuously making substantial contributions - to the project's codebase, review process, documentation and ADRs -* Have stake in the Cosmos SDK project, represented by: - * Being a client / user of the Cosmos SDK - * "[giving back](https://www.debian.org/social_contract)" to the software -* Delegate representation in case of vacation or absence - -Code owners need to maintain participation in the process, ideally as members of **Concept Approval Committee** -members, but at the very least as active participants in ADR discussions - -Removal criteria: - -* Missing 3 meetings results in ICF evaluating whether the member should be removed / replaced -* Violation of Code of Conduct - ### Implementation & Release Approval The following process should be adhered to both for implementation PRs corresponding to ADRs, as