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.

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

chore: Update code review guidelines #9307

Merged
merged 5 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions .github/reviewers.yml
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
reviewers:
groups:
developers:
- jmtaber129
reviewers:
- igorlesnenko
- nickoferrall
- BartoszJarocki
maintainers:
- mattkrick
- Dschoordsch
Expand All @@ -13,15 +11,14 @@ reviewers:
- tghanken
designers:
- ackernaut
- enriquesanchez
devops:
- rafaelromcar-parabol
- adaniels-parabol
- dbumblis-parabol
none:

per_author:
developers:
reviewers:
- none
maintainers:
- none
Expand All @@ -33,7 +30,7 @@ reviewers:
- none

defaults:
- developers
- maintainers

files:
"**/migrations/**":
Expand Down
28 changes: 6 additions & 22 deletions docs/codeReview.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
# Code review policy

The goal is to help reviewers to make their intentions clear to the author and for the author to know exactly what is expected of them to pass the next review round.
Review progress is tracked in the [Sprint Board](https://github.com/orgs/ParabolInc/projects/1) by the issue the PR belongs to.
If there is no issue belonging to the PR, then the PR itself should be added to the board.
A PR advances from self-review to reviewer-review and finally maintainer review before it can get merged.

## Submitting a PR

Expand All @@ -13,9 +10,10 @@ A PR advances from self-review to reviewer-review and finally maintainer review

## Requesting Code Reviews

- Self-assess which domains are involved in the changes made by your PR, find a reviewer from among the folks who fill the [Code Reviewer (L2–3)](https://www.notion.so/Code-Reviewer-L2-3-a47bb0759a0b41b5b0469ff14a8cdaae) role, paying specific attention to who GitHub suggests, who has indicated expertise within a domain (e.g. backend, front-end, etc.), and who has capacity to review within 24 hours.
- Self-assess which domains are involved in the changes made by your PR, find a reviewer from among the folks who are in the reviewers or maintainer [groups](/.github/reviewers.yml), paying specific attention to who GitHub suggests, who has indicated expertise within a domain (e.g. backend, front-end, etc.), and who has capacity to review within 24 hours.
- If you feel that maintainer review isn't necessary, add the label: "One Review Required". Use the label for PRs of any size and complexity if you are confident enough, and it feels safe. One exception: we should be extremely careful with DB migrations as they might break the application in the way that is hard to restore.
Copy link
Contributor

Choose a reason for hiding this comment

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

So if I understand correctly, basically, we decide if PR is complex or not and request either reviewer or maintainer. Do we need to keep One Review Required label in this case if anyway it always be a single review?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of "One Review Required" was to bypass maintainer review. I think that's still worth marking as it makes it easier to check after some time if it worked out. I'm open to get new name suggestions.

- Otherwise request a review directly from a maintainer.
- If the PR has not been reviewed within 24 hours, ping the reviewer on Slack and ask when they'll be able to review. If they don't have capacity for review, reassign to a different reviewer. If you're still blocked at the next check-in meeting, add an agenda item.
- If the developer creating the PR feels that maintainer review isn't necessary, they should add a label: “One Review Required”. Use the label for PRs of any size and complexity if you are confident enough, and it feels safe. One exception: we should be extremely careful with DB migrations as they might break the application in the way that is hard to restore.

## Performing Code Reviews

Expand All @@ -26,13 +24,6 @@ A PR advances from self-review to reviewer-review and finally maintainer review
- When reviewing a PR and requesting changes, be mindful that the PR author won't always have the right background to understand what are you requesting. Make your comments meaningful, and record a Loom if needed.
- [The right balance](https://docs.gitlab.com/ee/development/code_review.html#the-right-balance)

## Sprint Board

- Issues can go in these columns: To Prioritize, Backlog, To Do, In Progress, Stuck.
- Pull Requests can go in these columns: Stuck, Self Review, Reviewer Review, Maintainer Review.
- A PR may correpsond to 0 or many issues. These issues shall stay "In Progress" and when the PR gets merged, they'll be moved to "Done" automatically.
- The motivation for this structure is to relax the constraint that 1 issue has 1 PR. If a PR resolves 3 issues, we only have to update the status of the PR, not update all 3 issues as a group. If a PR fails to resolve 1 of the 3 issues, the remaining issue stays "In Progress" instead of being moved from "Maintainer Review" back to "In Progress". If an issue has multiple PRs, the issue can stay "In Progress" while the PRs move through the process.

## Reviewer

- Prefix each comment with a label. The labels are not points and will not be summed up or similar.
Expand All @@ -48,24 +39,17 @@ A PR advances from self-review to reviewer-review and finally maintainer review
e.g. nice work here, I learnt something, good find
- Final review is "Approve" if there are no negative comments, otherwise "Request changes"
- Changes requested? Move the issue associated with the PR to the self-review column.
- Approved? Move the associated issue to maintainer-review.

## Maintainer

- Maintainer follows the same process of Reviewers
- Approved by the maintainer?
- If there are +1 comments, the issue the PR belongs to goes back to self-review to give the author a chance to react to the comments.
- If there are only +2 comments, the PR can be merged directly by the maintainer.
- Approved? The PR can be merged by either the author or a maintainer

## Metrics Representative

- Metrics Representative ensures any analytics related changes work well with downstream data services
- This role is currently filled by @tianrunhe
- This role is currently filled by the [data group](/.github/reviewers.yml)

## Author

- Answer or resolve each comment
- resolve if you followed the suggestion
- reply if you didn't
- If you need to clarify parts of the code, check if it can be done by adding comments or improve naming of variables/functions/classes
- When you replied or resolved all comments, move the PR back to review
- When you replied or resolved all comments, re-request review so the PR shows up in the review requested search
Loading