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

Updated code review guidelines #376

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Updated code review guidelines #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20c1eb4
Updated code review guidelines.
trmartin4 Jun 29, 2024
3f8aec1
Updated code review and pull request from feedback
trmartin4 Jul 3, 2024
247aa75
Merge branch 'main' into update-review-process
trmartin4 Jul 3, 2024
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
48 changes: 46 additions & 2 deletions docs/contributing/pull-requests/code-review.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
---
sidebar_position: 2
sidebar_custom_props:
access: bitwarden
---

# Code Review Guidelines
# Code Review

At Bitwarden, we encourage everyone to participate in code reviews. A team will focus primarily on
their own code reviews, but if you see something interesting, feel free to jump in and discuss.

We believe that the act of reviewing PRs is a critically important part of each engineer's job. It
is as important, if not more important, than the act of writing code.

A few general guidelines:

- Pull requests should be manageable. If the PR is too large -- significantly above a few hundred
Expand All @@ -26,6 +31,33 @@ You can find more tips for PR review here:

:::

## Responding to review requests

To ensure that teams within the organization operate on same set of assumptions for performing
reviews, we have agreed to a baseline set of expectations.

When a PR author opens a PR for review, they should have the expectation that:

- The act of opening the PR for review is the **only** notification required. Teams are responsible
for properly configuring notifications so that team members are aware of their obligations.
- The reviewing team(s) will respond within **two business days** to:
- Provide a review,
- Inform the author when a review will be provided, or
- Ask the author to split the work into a
[smaller PR](./branching.md#structuring-branches-to-support-incremental-work) for review

:::tip Notifications

Our teams use GitHub notifications as the primary method of communication for PR review requests and
scheduled reminders are highly encouraged to facilitate prompt responses to requests.

- Individual engineers are encouraged to set up [scheduled reminders][user reminders] for
themselves.
- Each team has [scheduled reminders][team reminders] on a dedicated Slack channel (e.g.
#team-eng-platform-notifications).

:::

## Reviewing

If you feel that someone else has good knowledge of the code you are reviewing, please feel free to
Expand Down Expand Up @@ -115,9 +147,17 @@ claims. This can be based on testing the change or on previous domain knowledge.
- Solve the intended problem,
- [solve the requirements in the best way](#assumptions-note),
- the code is well structured,
- follows our most recent, accepted patterns,
- follows our most recent, accepted patterns as defined in our [ADRs](../../architecture/adr/),
- and is free of unintended side-effects.

:::warning Evolutionary Database Design

For any database changes, be sure that they follow
[EDD](../../contributing/database-migrations/edd.mdx). This is important as the lack of EDD support
will **not** be caught by any unit, integration, or regression testing.

:::

If you are unsure about any of the above, consider using a different status or check in with the
author to discuss things first. Also don’t hesitate to request a second review from someone else.

Expand Down Expand Up @@ -183,6 +223,10 @@ without managing remote branches - for example:
gh pr checkout <GitHub PR number>
```

[user reminders]:
https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-membership-in-organizations/managing-your-scheduled-reminders
[team reminders]:
https://docs.github.com/en/organizations/organizing-members-into-teams/managing-scheduled-reminders-for-your-team
[sme-yellowpages]: https://bitwarden.atlassian.net/wiki/spaces/DEV/pages/195919928
[gh-commenting]:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request
Expand Down
90 changes: 28 additions & 62 deletions docs/contributing/pull-requests/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ sidebar_position: 4

# Pull Requests

:::success Reviewing Pull Requests

This page focuses on authoring and addressing feedback on Pull Requests. For details and
expectations for PR reviewers, see [Code Review](./code-review.md).

:::

Pull Requests are the primary mechanism we use to write software. GitHub has some great
[documentation](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests)
on using the Pull Request feature.
Expand Down Expand Up @@ -122,70 +129,22 @@ assigned teams to review it. If the changes are still in progress, leave the PR
Doing this ensures that reviewers can act on the "Ready for Review" as their signal to begin the
review process without further notification.

### Addressing feedback

It is likely that you will receive some feedback on your PR. You should see this as a positive thing
-- it signifies a healthy and thorough review process and an organizational commitment to code
quality. You may receive [comments](./code-review.md#comment) or a
[request for changes](./code-review.md#request-changes). You are encouraged to engage in
conversation on the PR to discuss a solution, but if any strong conflicting opinions arise it is
often best to move the conversation to a synchronous format to avoid any misunderstanding.

When any necessary changes have been made, you should address the comments or request for changes by
responding in the PR conversation thread. You are not responsible for resolving the conversation --
that is the prerogative of the reviewer, to ensure that they agree that the question or concern has
been addressed.

**When you are ready for a reviewer to revisit your changes, you should request a re-review.** This
will notify the reviewer and ensure a prompt response.

</Bitwarden>

## Reviewing the pull request

<Bitwarden>

At Bitwarden, we believe that the act of reviewing PRs is a critically important part of each
engineer's job. It is as important, if not more important, than the act of writing code.

To ensure that teams within the organization operate on same set of assumptions for performing
reviews, we have agreed to a baseline set of expectations.

When a PR author opens a PR for review, they should have the expectation that:

- The act of opening the PR for review is the **only** notification required. Teams are responsible
for properly configuring notifications so that team members are aware of their obligations.
- The reviewing team(s) will respond within **2 business days** to:
- Provide a review,
- Inform the author when a review will be provided, or
- Ask the author to split the work into a
[smaller PR](./branching.md#structuring-branches-to-support-incremental-work) for review

:::tip Notifications

Our teams use GitHub notifications as the primary method of communication for PR review requests and
scheduled reminders are highly encouraged to facilitate prompt responses to requests.

- Individual engineers are encouraged to set up [scheduled reminders][user reminders] for
themselves.
- Each team has [scheduled reminders][team reminders] on a dedicated Slack channel (e.g.
#team-eng-platform-notifications).

:::
You should receive a review or at least first contact from a reviewer from each assigned team within
**two business days** of marking the PR as Ready for Review.

### Follow-up notification

If there is no response to a request for review in 48 hours, the author should reach out to the
team(s) -- or to individual engineers if assigned -- via Slack to follow up.
If there is no response to a request for review for two business days, the author should reach out
to the team(s) -- or to individual engineers if assigned -- via Slack to follow up.

This should wait for 48 hours to allow the default process to take place and not overwhelm the team
with notifications on multiple platforms.
This should wait for two business days to allow the default process to take place and not overwhelm
the team with notifications on multiple platforms.

:::warning Urgent reviews

If deployment deadlines or other concerns mean there is a need to shorten the review period for a
pull request, the author should reach out to the reviewing team via their Slack channel. This should
**only** be necessary for urgent requests and for follow-up after 48 hours.
**only** be necessary for urgent requests and for follow-up after two business days.

:::

Expand All @@ -199,12 +158,19 @@ that may delay this process.

</Community>

### How to perform a review
### Addressing feedback

It is likely that you will receive some feedback on your PR. You should see this as a positive thing
-- it signifies a healthy and thorough review process and an organizational commitment to code
quality. You may receive [comments](./code-review.md#comment) or a
[request for changes](./code-review.md#request-changes). You are encouraged to engage in
conversation on the PR to discuss a solution, but if any strong conflicting opinions arise it is
often best to move the conversation to a synchronous format to avoid any misunderstanding.

We've written up some [guidelines](./code-review.md) for reviewing code, which we recommend reading
before performing your first code review.
When any necessary changes have been made, you should address the comments or request for changes by
responding in the PR conversation thread. You are not responsible for resolving the conversation --
that is the prerogative of the reviewer, to ensure that they agree that the question or concern has
been addressed.

[user reminders]:
https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-your-membership-in-organizations/managing-your-scheduled-reminders
[team reminders]:
https://docs.github.com/en/organizations/organizing-members-into-teams/managing-scheduled-reminders-for-your-team
**When you are ready for a reviewer to revisit your changes, you should request a re-review.** This
will notify the reviewer and ensure a prompt response.