Updates to reflect best practices for PR review #220
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| --- | ||
| sidebar_position: 1 | ||
| sidebar_custom_props: | ||
| access: bitwarden | ||
| --- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| --- | ||
| sidebar_position: 3 | ||
| sidebar_custom_props: | ||
| access: bitwarden | ||
| --- | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,33 +97,106 @@ changes” to a PR forcing the reviewer to start over again. | |||||
|
|
||||||
| ::: | ||||||
|
|
||||||
| ## Creating a pull request | ||||||
|
|
||||||
| The Bitwarden repositories have a _Pull Request template_ which should be followed. This will ensure | ||||||
| the PR review goes smoothly since it will provide context to the reviewer.<community> Once a | ||||||
| community PR has been created, it will be automatically be linked to an internal Jira ticket. The | ||||||
| internal ticket is used for prioritization and tracking purposes.</community><bitwarden> When | ||||||
| creating the PR include a Jira ticket reference so the reviewer can gain all context on the work as | ||||||
| well as links to any associated PRs in other repositories.</bitwarden> | ||||||
|
|
||||||
| <bitwarden> | ||||||
|
|
||||||
| ### Tagging reviewers | ||||||
|
|
||||||
| We use `CODEOWNERS` in each repository to assign the reviewing teams based on the files in the PR. | ||||||
| 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 | ||||||
| assigned teams to review it. If the changes are still in progress, leave the PR in `Draft` status. | ||||||
| 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. | ||||||
|
Comment on lines
+136
to
+138
There was a problem hiding this comment. This is also a significant clarification (different people do different things at the moment) - would like this highlighted when these changes are shared |
||||||
|
|
||||||
| **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 | ||||||
|
There was a problem hiding this comment. Do we want to leave a note somewhere about re-requesting review once the feedback is resolved? I've noticed it's sometimes not done which delays new reviews. There was a problem hiding this comment. Also support. This helps a lot, especially when many teams are involved in a review. There was a problem hiding this comment. I created a section called "Addressing feedback" that includes this, as well as a recommendation to allow the commenter to "Resolve conversation" - if that's not our position I can remove that part. |
||||||
|
|
||||||
| <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. | ||||||
|
Comment on lines
+157
to
+158
There was a problem hiding this comment. I would want us to document some available integrations, and give teams a chance to start using them, before actively enforcing it. There was a problem hiding this comment. Handy link on more info: https://docs.github.com/en/enterprise-cloud@latest/organizations/organizing-members-into-teams/managing-code-review-settings-for-your-team I don't know if we need anything more than what GitHub provides. Maybe Slack reminders? There was a problem hiding this comment. I get 30+ notifications daily which makes GH notifications mostly unusable without spending a non trivial amount of time maintaining it. There was a problem hiding this comment. I've added a section for this: My assumption was that the build-in reminder integration with Slack was sufficient and we didn't need to customize anything, but that may not be the case. There was a problem hiding this comment. I expect that removing tech-leads as the default reviewer should help with notification overload. |
||||||
| - The reviewing team(s) will respond within **24 hours** 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 for review | ||||||
|
There was a problem hiding this comment. Has the team agreed to a 24 hour PR review deadline? I haven't seen any discussion about this, but I may have missed it. There was a problem hiding this comment. Not to my knowledge. I'm not against this though but it should be communicated. I also think we need to spend further effort to clean up tech-leads codeowners since I haven't found a way to exclude them from review filters. There was a problem hiding this comment. I agree. I wanted to discuss here and then communicate in a bulleted list to the teams in Slack. This felt like a good place to start the discussion. I’m not in any way pushing for 24 - if we want to have a longer timeframe that’s fine. My main concern is having everyone bought in on what the timeframe is so we don’t have to resort to pinging on Slack for every PR. There was a problem hiding this comment. Maybe "business day"? I personally feel one day is actually pretty tough unless engineers can be in formal support rotations and have this as a priority. There was a problem hiding this comment. I'm not sure what would be a reasonable deadline here, although I agree 1 business day is tight. 2 business days? This is not a very visible place for discussion and this would be a fairly major change to the current expectations, I suggest sharing in Slack. There was a problem hiding this comment. After discussion with Tech Leads and EMs, we settled on 48 hours. I've addressed that in 3131dcf There was a problem hiding this comment. issue: This line needs to be expanded upon and supported by a section informing the author on how to create PRs for an intended change. This could be added to the branching document and linked here. I think this is the primary reason for our large PR backlog. My recommendation would be limiting PRs to 1000 loc and preferring config or software feature flagging to allow for incomplete features to be present in the codebase.
Suggested change
There was a problem hiding this comment. @MGibson1, I've addressed this by completely revamping the branching page, in 440c84d. As we are moving closer to a trunk-based branching scheme, with the preference toward merging to I also thought that something like this might be nice to have: https://github.com/marketplace/actions/pull-request-size-labeler There was a problem hiding this comment. I think you nailed it, thank you. I'm not really that convinced that the labels would have value, but the concept certainly does. If you think that'll help encourage people to smaller bites let's do it! |
||||||
|
|
||||||
| :::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). | ||||||
|
|
||||||
| ::: | ||||||
|
|
||||||
| ### Follow-up notification | ||||||
|
|
||||||
| If there is no response to a request for review in 24 hours, the author should reach out to the | ||||||
| team(s) -- or to individual engineers if assigned -- via Slack to follow up. | ||||||
|
|
||||||
| This should wait for 24 hours to allow the default process to take place and not overwhelm the team | ||||||
| with notifications on multiple platforms. | ||||||
|
|
||||||
| </bitwarden> | ||||||
|
|
||||||
| <community> | ||||||
|
|
||||||
| Once a Community PR has been created a Bitwarden developer will perform a code review. While we try | ||||||
| to this in a reasonable time frame, please understand that we have internal roadmaps and priorities | ||||||
| that may delay this process. | ||||||
|
|
||||||
| </community> | ||||||
|
|
||||||
| ### How to perform 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]: | ||||||
|
There was a problem hiding this comment. ℹ️ I didn't know these links could have spaces in them and they're usually shorthand, but if it works ... |
||||||
| 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 | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting sidebar positions on these? Do we dislike the alphabetical ordering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't ordering alphabetically before - the positions are to order it alphabetically. I'll take them out and double-check that I'm correct in that statement.