Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added PR Review Policy #16280

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Added PR Review Policy #16280

merged 3 commits into from
Jun 15, 2023

Conversation

Samyoul
Copy link
Member

@Samyoul Samyoul commented Jun 14, 2023 •
edited
Loading

What's changed

I've written up a policy document following from the great Council and Mauve Council of Deadwater.

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:

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.

Human readable version is here

Why Change

So that we have a PR review policy that brings balance

@Samyoul Samyoul added the docs label Jun 14, 2023
@Samyoul Samyoul self-assigned this Jun 14, 2023
@Samyoul Samyoul requested review from vkjr, ilmotta, cammellos, rasom, flexsurfer and siddarthkay and removed request for vkjr June 14, 2023 15:02
@status-im-auto
Copy link
Member

status-im-auto commented Jun 14, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (16)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ e4b5747 #1 2023-06-14 15:07:03 ~6 min android-e2e 🤖apk 📲
✔️ e4b5747 #1 2023-06-14 15:07:06 ~6 min android 🤖apk 📲
✔️ e4b5747 #1 2023-06-14 15:09:31 ~8 min tests 📄log
✔️ e4b5747 #1 2023-06-14 15:09:58 ~9 min ios 📱ipa 📲
✔️ 36d6332 #2 2023-06-14 21:01:15 ~5 min android-e2e 🤖apk 📲
✔️ 36d6332 #2 2023-06-14 21:01:37 ~6 min android 🤖apk 📲
✔️ 36d6332 #2 2023-06-14 21:01:59 ~6 min ios 📱ipa 📲
✔️ 36d6332 #2 2023-06-14 21:03:40 ~8 min tests 📄log
✔️ 379a835 #4 2023-06-14 21:12:30 ~5 min ios 📱ipa 📲
✔️ 379a835 #4 2023-06-14 21:12:52 ~6 min android-e2e 🤖apk 📲
✔️ 379a835 #4 2023-06-14 21:13:15 ~6 min android 🤖apk 📲
✔️ 379a835 #4 2023-06-14 21:14:04 ~7 min tests 📄log
✔️ 7d46c90 #5 2023-06-15 13:41:57 ~6 min android-e2e 🤖apk 📲
✔️ 7d46c90 #5 2023-06-15 13:42:15 ~6 min ios 📱ipa 📲
✔️ 7d46c90 #5 2023-06-15 13:42:39 ~6 min android 🤖apk 📲
✔️ 7d46c90 #5 2023-06-15 13:48:46 ~12 min tests 📄log
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 33986cf #6 2023-06-15 14:35:00 ~5 min android-e2e 🤖apk 📲
✔️ 33986cf #6 2023-06-15 14:35:35 ~6 min ios 📱ipa 📲
✔️ 33986cf #6 2023-06-15 14:36:57 ~7 min android 🤖apk 📲
❌ 33986cf #6 2023-06-15 14:37:51 ~8 min tests 📄log
❌ 33986cf #7 2023-06-15 14:55:07 ~4 min tests 📄log
✔️ 325d0a8 #7 2023-06-15 19:48:24 ~5 min android 🤖apk 📲
✔️ 325d0a8 #7 2023-06-15 19:49:10 ~6 min ios 📱ipa 📲
✔️ 325d0a8 #7 2023-06-15 19:49:24 ~6 min android-e2e 🤖apk 📲
✔️ 325d0a8 #8 2023-06-15 19:51:27 ~8 min tests 📄log

J-Son89
J-Son89 approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@J-Son89 J-Son89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

OmarBasem
OmarBasem approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@OmarBasem OmarBasem left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good to me.
Just one thing, if it is possible to make the "policy" part a bit more concise would be better I think. (there are in total 18 main and sub-points -- people may skip reading them)

@Samyoul Samyoul requested a review from ibrkhalil June 14, 2023 16:22
@Samyoul
Copy link
Member Author

Samyoul commented Jun 14, 2023

@OmarBasem That is a good point, I'll make a change to highlight the main 3 points of the policy. Something like:

Main points

  • 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 code owner has the responsibility for getting a PR reviewed and approved.
    1. The code owner has the right and responsibility to pursue code reviews
    2. The code owner has the right and responsibility to make multiple requests of the same reviewer
    3. The code owner will select a limited number of reviewers to review any of the code owner’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 code owner 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
  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. Don’t do it.
  6. All team members acknowledge that without acting with honesty this review process will not work and jeopardises their future ability to receive reviews.

ajayesivan
ajayesivan reviewed Jun 14, 2023
View reviewed changes
doc/pr-review-policy.md Outdated

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 preview load. Because of this potential damage a breach of the honour system is considered a deeply shameful act.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR preview? is this a typo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thank you. That is a tpyo. I will amend

ajayesivan
ajayesivan approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@ajayesivan ajayesivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well documented 🫡

ilmotta
ilmotta approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, such a nice and well written doc in the spirit of a true open-source project. Thank you @Samyoul

@Samyoul Samyoul force-pushed the doc/pr-review-policy branch 2 times, most recently from 327d782 to 379a835 Compare June 14, 2023 21:06
siddarthkay
siddarthkay approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@siddarthkay siddarthkay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this initiative @Samyoul !

doc/pr-review-policy.md Outdated Show resolved Hide resolved
@Samyoul Samyoul merged commit c9c7b53 into develop Jun 15, 2023
@Samyoul Samyoul deleted the doc/pr-review-policy branch June 15, 2023 22:02
codemaster115 pushed a commit that referenced this pull request Jun 16, 2023
@Samyoul @codemaster115
Added PR Review Policy (#16280)
04d2d9a 
* Added PR Review Policy

* Reformat of Policy section to headline with policy fundementals

* Renaming code owner to requester
codemaster115 pushed a commit that referenced this pull request Jul 7, 2023
@Samyoul @codemaster115
Added PR Review Policy (#16280)
55f7807 
* Added PR Review Policy

* Reformat of Policy section to headline with policy fundementals

* Renaming code owner to requester
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siddarthkay siddarthkay siddarthkay approved these changes

@ibrkhalil ibrkhalil ibrkhalil approved these changes

@OmarBasem OmarBasem OmarBasem approved these changes

@J-Son89 J-Son89 J-Son89 approved these changes

@ajayesivan ajayesivan ajayesivan approved these changes

@vkjr vkjr vkjr approved these changes

@rasom rasom rasom approved these changes

@ilmotta ilmotta ilmotta approved these changes

@cammellos cammellos Awaiting requested review from cammellos

@flexsurfer flexsurfer Awaiting requested review from flexsurfer

Assignees

@Samyoul Samyoul

Labels
docs
Projects
No open projects
Legacy QA Automation Project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Samyoul @status-im-auto @ilmotta @rasom @vkjr @ajayesivan @J-Son89 @OmarBasem @ibrkhalil @siddarthkay