From 20c1eb4122eaf69b51da730a7827095bb0ad0f77 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Fri, 28 Jun 2024 21:36:09 -0400 Subject: [PATCH 1/2] Updated code review guidelines. --- .../contributing/pull-requests/code-review.md | 10 ++++- docs/contributing/pull-requests/index.md | 42 +++++++++---------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/docs/contributing/pull-requests/code-review.md b/docs/contributing/pull-requests/code-review.md index 174c665f..803df2af 100644 --- a/docs/contributing/pull-requests/code-review.md +++ b/docs/contributing/pull-requests/code-review.md @@ -115,9 +115,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. diff --git a/docs/contributing/pull-requests/index.md b/docs/contributing/pull-requests/index.md index 0e6abf84..670cca46 100644 --- a/docs/contributing/pull-requests/index.md +++ b/docs/contributing/pull-requests/index.md @@ -115,6 +115,8 @@ These reviews are required for the changes to be merged to `master`. You can tag additional teams or individuals for review as you see fit, including `@dept-design` for any design changes. + + ### Opening the PR for review As its name implies, marking a PR as "Ready for Review" indicates that you are ready for all @@ -122,26 +124,7 @@ 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. - - - -## Reviewing the pull request +## Managing the review process @@ -199,7 +182,24 @@ that may delay this process. -### 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. + +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. + +## Performing a review We've written up some [guidelines](./code-review.md) for reviewing code, which we recommend reading before performing your first code review. From 3f8aec1c356cc2c4c35e297f8adcc92ca5baaa69 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Tue, 2 Jul 2024 22:00:03 -0400 Subject: [PATCH 2/2] Updated code review and pull request from feedback --- .../contributing/pull-requests/code-review.md | 38 +++++++++++- docs/contributing/pull-requests/index.md | 62 +++++-------------- 2 files changed, 51 insertions(+), 49 deletions(-) diff --git a/docs/contributing/pull-requests/code-review.md b/docs/contributing/pull-requests/code-review.md index 803df2af..1591237f 100644 --- a/docs/contributing/pull-requests/code-review.md +++ b/docs/contributing/pull-requests/code-review.md @@ -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 @@ -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 @@ -191,6 +223,10 @@ without managing remote branches - for example: gh pr checkout ``` +[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 diff --git a/docs/contributing/pull-requests/index.md b/docs/contributing/pull-requests/index.md index 670cca46..319e8ef7 100644 --- a/docs/contributing/pull-requests/index.md +++ b/docs/contributing/pull-requests/index.md @@ -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. @@ -115,8 +122,6 @@ These reviews are required for the changes to be merged to `master`. You can tag additional teams or individuals for review as you see fit, including `@dept-design` for any design changes. - - ### Opening the PR for review As its name implies, marking a PR as "Ready for Review" indicates that you are ready for all @@ -124,51 +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. -## Managing the review process - - - -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. ::: @@ -198,13 +174,3 @@ 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. - -## Performing a review - -We've written up some [guidelines](./code-review.md) for reviewing code, which we recommend reading -before performing your first code review. - -[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