From e50fea9dff7bfd873bc5ca289575dbb49ac6276c Mon Sep 17 00:00:00 2001 From: amyblais Date: Thu, 1 Feb 2018 07:00:32 -0500 Subject: Update CONTRIBUTING.md (#8168) --- CONTRIBUTING.md | 70 +++++++++++++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 29 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b9430acfe..6ac9356c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,51 +13,63 @@ The one exception may be around release time, where the review process may take After a PR is submitted, a core committer applies labels and notifies product managers (PMs) that there is a PR awaiting review by posting in the [PM/Docs PR Review channel](https://pre-release.mattermost.com/core/channels/pmdocs-pr-review-pub), which is a channel to discuss community pull requests that need review by PMs. Then, one or more of the labels is applied: - - `Awaiting PR`: Applied if the PR is awaiting another to be merged. For example, when a client PR is awaiting a server PR to be merged first. Once the PR is no longer blocked, the core committer removes the `Awaiting PR` label - - `1: PM Review`: Applied if the PR has UI changes or functionality that PMs can test on test servers called "spinmints" - - `Major Change`: Applied if the PR is a major feature or affects large areas of the code base, e.g. [moving channel store and actions to Redux](https://github.com/mattermost/platform/pull/6235) - - `Setup Test Server`: Applied if the PR is queued for PM testing - - `Work in Progress`: Applied if the PR is unfinished and needs further work before it's ready for review + - `Awaiting PR`: Applied if the PR is awaiting another to be merged. For example, when a client PR is awaiting a server PR to be merged first. Once the PR is no longer blocked, the core committer removes the `Awaiting PR` label. + - `1: PM Review`: Applied if the PR has UI changes or functionality that PMs can test on test servers called "spinmints". + - `Major Change`: Applied if the PR is a major feature or affects large areas of the code base, e.g. [moving channel store and actions to Redux](https://github.com/mattermost/platform/pull/6235). + - `Setup Test Server`: Applied if the PR is queued for PM testing. + - `Work in Progress`: Applied if the PR is unfinished and needs further work before it's ready for review. #### Stage 1: PM Review -A product manager will review the pull request to make sure it: +A product manager (PM) will review the pull request to make sure it: + - Fits with our product roadmap. + - Works as described in the ticket. + - Meets [user experience guidelines](https://docs.mattermost.com/developer/fx-guidelines.html). - - Fits with our product roadmap - - Works as described in the ticket - - Meets [user experience guidelines](https://docs.mattermost.com/developer/fx-guidelines.html) +This step is sometimes skipped for bugs or small improvements with a well-defined ticket. In this case the core committer will assign the appropriate devs for review. A PM can also ask developer support to set up a separate test instance if the PR cannot be easily tested. -This step is sometimes skipped for bugs or small improvements with a well defined ticket. +When the review process begins: + - Mattermost Core Committer + - Assigns `1: PM Review` label within 1 business day after the PR is submitted. + - Assigns PM reviewer under Assignees. Those related to end user features are assigned to @esethna, others to @jasonblais. PM re-assigns as needed. + - PM + - Applies milestone for next release if the PM thinks there is enough time for the PR to be merged and sufficiently tested on `master` before code complete. Otherwise moved to a future release, letting submitter know that PR may have a delay in review due to the release cycle. + - Follows up with contributor if the CLA has not been signed. If no response within 7 days, PM closes the issue. + - PM verifies there is a corresponding JIRA ticket or GitHub issue. -When the review process begins, the PM applies a milestone: - - Set for next release if the PM thinks there is enough time for the PR to be merged and sufficiently tested on `master` before code complete. - - Set for a future release if PR is too large to test prior to the code complete date - - PM responds to submitter letting them know that PR may have a delay in review due to the release cycle - -Next, the PM tests changes on the spinmint: +Next, the PM tests changes on a spinmint test server: + - PM tests and verifies pull requests via the `Setup Test Server` label. Initial review is completed within 2 business days. - If changes are required, PM submits review as "Changes Requested", with a comment on the areas that require updates. Comment explains why changes are needed linking back to design principles. - - PM applies `Awaiting Submitter Action` label to more easily query the PR queue + - PM applies `Awaiting Submitter Action` label to more easily query the PR queue. - Once changes are made, PM regenerates test server and repeats testing. - If bugs are found that are also on `master`, a new bug report is submitted in JIRA and linked to the PR. Bugs that are also found on `master` will typically not block merging of PRs. - - If PR is approved, PM submits review as "Approved" commenting with areas that were tested. Then: - - PM removes `1: PM Review` and `Setup Test Server` labels - - PM applies the `Stage 2: Dev Review` label, which moves the PR to Stage 2 - + - If PR is approved, PM submits review as "Approved", commenting with areas that were tested. + #### Stage 2: Dev Review -Two developers will review the pull request and either give feedback or approve the PR. If changes are required: - - Dev submits review as "Changes Requested", with a comment on the areas that require tweaks. - - Once changes are made, dev reviews code changes +Two developers will review the pull request and either give feedback or approve the PR. Any comments should be addressed before the pull request moves on to the next stage. -Any comments should be addressed before the pull request moves on to the last stage. + - PM reviewer adds `2: Dev Review` label, removes `1: PM Review` and `Setup Test Server` labels, and assigns 2 developers for review under Reviewers. + - At least one dev is assigned based on their [feature area](https://docs.mattermost.com/developer/core-developer-handbook.html#current-core-developers). Devs re-assign as needed. + - Devs review the code and provide feedback, with initial review completed within 2 business days. Some areas to check include: + - Proper Unit Tests + - API documentation + - Localization + - After the submitter has addressed and satisfied all reviewers' comments, `3: Ready to Merge` label is applied. #### Stage 3: Ready to Merge -The review process is complete, and the pull request will be merged. +Review process is complete and the pull request is merged. + + - Dev assigns `3: Ready to Merge` label. + - If Mattermost is not in release mode (between [major feature cut and release candidate cut](https://docs.mattermost.com/process/release-process.html), the PR is merged into `master`. + - If the PR is a major change, merge is postponed until the next release cycle. + - Dev calls out on the issue that it is a major change and it will be merged after branching. + - Once the current release is branched the PR can be merged into `master`. #### PR Merged After a PR is merged: -- External Contributions: PM closes the [Help Wanted] issue and related Jira ticket -- Internal Contributions: Core committer resolves the JIRA ticket -- PM follows up for docs, changelog and release tests when working through the PR tracking spreadsheet +- External Contributions: PM closes the [Help Wanted] issue and related JIRA ticket. +- Internal Contributions: Core committer resolves the JIRA ticket. +- PM follows up for docs and changelog, and QA for release tests. -- cgit v1.2.3-1-g7c22