Added PR Review Policy (#16280)
* Added PR Review Policy * Reformat of Policy section to headline with policy fundementals * Renaming code owner to requester
This commit is contained in:
parent
42040e4fc9
commit
c9c7b538c7
|
@ -16,6 +16,8 @@
|
||||||
|
|
||||||
[Merging PR process](merging-pr-process.md)
|
[Merging PR process](merging-pr-process.md)
|
||||||
|
|
||||||
|
[PR Review Policy](pr-review-policy.md)
|
||||||
|
|
||||||
[Working on PR together with QA team](pipeline_process.md)
|
[Working on PR together with QA team](pipeline_process.md)
|
||||||
|
|
||||||
[Debugging](debugging.md)
|
[Debugging](debugging.md)
|
||||||
|
|
|
@ -5,6 +5,7 @@
|
||||||
3) Address feedback
|
3) Address feedback
|
||||||
4) Make sure builds and tests are green (run `make test` locally, `make lint-fix` to fix any indentation issue and `make lint`)
|
4) Make sure builds and tests are green (run `make test` locally, `make lint-fix` to fix any indentation issue and `make lint`)
|
||||||
5) Once the PR has been reviewed by the dev team you can run e2e tests on it by going to https://github.com/status-im/status-mobile/projects/7 and move the pr under the column E2E tests. This will trigger tests.
|
5) Once the PR has been reviewed by the dev team you can run e2e tests on it by going to https://github.com/status-im/status-mobile/projects/7 and move the pr under the column E2E tests. This will trigger tests.
|
||||||
|
1) Read our [PR Review Policy](pr-review-policy.md) for more details on our approach to PR reviews.
|
||||||
6) Once e2e tests have run, they will report the result on the PR, if it's less than 100%, ask QA to take a look to make sure everything is in order (some might fail for legitimate reasons)
|
6) Once e2e tests have run, they will report the result on the PR, if it's less than 100%, ask QA to take a look to make sure everything is in order (some might fail for legitimate reasons)
|
||||||
7) Ask QA for manual testing if the PR requires it
|
7) Ask QA for manual testing if the PR requires it
|
||||||
8) Once it has been tested successfully, squash everything into one commit. rebase and merge. The commands we use:
|
8) Once it has been tested successfully, squash everything into one commit. rebase and merge. The commands we use:
|
||||||
|
|
|
@ -0,0 +1,96 @@
|
||||||
|
## Pull Request Review Policy
|
||||||
|
|
||||||
|
This document details the pull request policy of the Status Mobile repository / team.
|
||||||
|
|
||||||
|
- [Terms](#terms)
|
||||||
|
- [Policy](#policy)
|
||||||
|
- [Overview](#overview)
|
||||||
|
- [Full Policy](#fully-policy)
|
||||||
|
- [Rationale](#rationale)
|
||||||
|
- [Front pressure and Demand control](#front-pressure-and-demand-control)
|
||||||
|
- [Back pressure and Supply control](#back-pressure-and-supply-control)
|
||||||
|
- [Honour System](#honour-system)
|
||||||
|
- [Review Support](#review-support)
|
||||||
|
- [Background](#background)
|
||||||
|
|
||||||
|
### Terms:
|
||||||
|
|
||||||
|
| Term | Definition |
|
||||||
|
|----------------------|---------------------------------------------------------------------------------------------------------------|
|
||||||
|
| **Requester** | The team member submitting a pull request. |
|
||||||
|
| **Reviewer** | A team member asked by a requester to review the requester’s pull request. |
|
||||||
|
| **Review bandwidth** | The number of reviews a team member has self-limited themselves to giving within their defined review period. |
|
||||||
|
| **Review period** | The period of time over which a reviewer’s bandwidth is consumed |
|
||||||
|
|
||||||
|
### Policy:
|
||||||
|
|
||||||
|
#### Overview
|
||||||
|
|
||||||
|
- 🔀 - You are responsible for getting your PR reviewed and approved.
|
||||||
|
- ✅ - You must give a review on a PR if you are asked to do so, unless your review limit has been reached.
|
||||||
|
- ✨ - The honour system underpins this policy, without acting in good faith it will not work.
|
||||||
|
|
||||||
|
#### Fully policy
|
||||||
|
|
||||||
|
1) The requester has the responsibility for getting a PR reviewed and approved.
|
||||||
|
1) The requester has the right and responsibility to pursue code reviews.
|
||||||
|
2) The requester has the right and responsibility to make multiple requests of the same reviewer.
|
||||||
|
3) The requester will select a limited number of reviewers to review any of the requester’s PRs.
|
||||||
|
2) Reviewers are obligated to give a review for all PRs they are requested to review, except when their review bandwidth has been exhausted:
|
||||||
|
1) Reviewers have the right to refuse a requester a review if the reviewer’s review bandwidth has been exhausted.
|
||||||
|
2) Reviewers are expected to manage and track their review bandwidth.
|
||||||
|
3) Reviewers can manage their review bandwidth via any means they feel works best for them. Examples include:
|
||||||
|
1) Using the GitHub busy feature
|
||||||
|
2) Declining a review on the PR
|
||||||
|
3) Declining a review when pursued by a code reviewer
|
||||||
|
4) Proactively declining to review a PR
|
||||||
|
3) All team members will set a review bandwidth that is reasonable.
|
||||||
|
4) All team members acknowledge that giving reviews is a requirement of membership of the team.
|
||||||
|
5) All team members will manage their review bandwidth honestly.
|
||||||
|
1) The right to refuse a PR review is only eligible if a reviewer’s review bandwidth has actually been exhausted.
|
||||||
|
2) Breaching the team’s honour is very bad form. Do not do it (please).
|
||||||
|
6) All team members acknowledge that without acting with honesty this review process will not work and jeopardises their own future ability to receive reviews.
|
||||||
|
|
||||||
|
### Rationale
|
||||||
|
|
||||||
|
#### Front pressure and Demand control
|
||||||
|
|
||||||
|
Foremost in the rationale for the above policy is that giving a PR review is a requirement. If a requester requests
|
||||||
|
a PR review of a team member, the reviewer is now obligated to review the PR within a reasonable timeframe.
|
||||||
|
|
||||||
|
This principle enforces that there is a pressure and demand that PR reviews must be given.
|
||||||
|
|
||||||
|
#### Back pressure and Supply control
|
||||||
|
|
||||||
|
Front pressure alone is not enough to balance the PR review load across the team. Because of this an exception is granted
|
||||||
|
to reviewers that have exhausted their review bandwidth, the exemption permits these reviewers to refuse a PR review.
|
||||||
|
|
||||||
|
This principle gives the reviewer pool back-pressure, relieving the exhausted reviewer and increasing pressure / demand on
|
||||||
|
the other reviewers.
|
||||||
|
|
||||||
|
#### Honour system
|
||||||
|
|
||||||
|
The two principles of demand and supply control are underpinned by an honour system and are useless without honest action.
|
||||||
|
If a team member does not honestly manage their review bandwidth this action seriously damages the collective balance of
|
||||||
|
PR review load. Because of this potential damage a breach of the honour system is considered a deeply shameful act.
|
||||||
|
|
||||||
|
The honour system is the mechanism of control and enforcement, without it the policy would need to be controlled and enforced
|
||||||
|
using less dynamic and forgiving mechanisms. An example is a GitHub review counting tools with quotas and metrics. So just
|
||||||
|
be cool.
|
||||||
|
|
||||||
|
### Review Support
|
||||||
|
|
||||||
|
Sometimes reviewing is time-consuming or generally painful, to help make reviews as simple as possible consider the following suggestions:
|
||||||
|
- Paired reviews: Ask an experienced reviewer to review with you.
|
||||||
|
- If a PR is the product of multiple developers only ask 1 or 2 other developers for their review.
|
||||||
|
|
||||||
|
### Background
|
||||||
|
|
||||||
|
The contents of this policy document is the product of 2 councils of the Status Mobile team, the below documents summarise the topics and outcomes of the councils:
|
||||||
|
|
||||||
|
- [2023-05-15 - Council of Deadwater](https://docs.google.com/document/d/1TtUZCke2OzumjdWGu6TIECHEOUPwvaLrvQ8Re2nEGCo)
|
||||||
|
- [2023-06-13 - Mauve Council of Deadwater](https://docs.google.com/document/d/19hfr8xXrShLk9ROdQfOwb9_vwbTah9nOCqKSLIrgoSI)
|
||||||
|
|
||||||
|
The aims of the council were to allow developers that are relied on heavily for reviews to better balance their time
|
||||||
|
between reviews and other responsibilities. Equally, the council aimed to give other team members better opportunity and
|
||||||
|
understanding that giving PR reviews is a requirement and will help the team generally if more review burden is shared.
|
Loading…
Reference in New Issue