Updates to reflect best practices for PR review #220
Conversation
Deploying with
|
| Latest commit: |
815208e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6b2b42a4.contributing-docs.pages.dev |
| Branch Preview URL: | https://pr-review-updates.contributing-docs.pages.dev |
|
No New Or Fixed Issues Found |
| 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 **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
| @@ -1,4 +1,5 @@ | |||
| --- | |||
| sidebar_position: 1 | |||
There was a problem hiding this comment.
Why are we setting sidebar positions on these? Do we dislike the alphabetical ordering?
There was a problem hiding this comment.
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.
| We shouldn’t hesitate to use this status. However, it does come with obligations for the reviewer. | ||
| By blocking the PR from progressing, they have taken an ownership stake in it and should make | ||
| themselves available to answer clarifying questions. They should also give clear feedback on what | ||
| needs to change for the PR to get approved. Likewise, a PR author should not be discouraged by a | ||
| request for changes, it's simply an indication that changes should be made prior to the PR being | ||
| merged. This is common. |
There was a problem hiding this comment.
I would argue that every review comes with this obligations. I'm worried this might associate request changes negatively.
There was a problem hiding this comment.
Can you elaborate? Not sure why this language change does anything more than expect the reviewer to stay engaged.
I too want to be sure "request changes" is taken very objectively.
There was a problem hiding this comment.
"However, it does come with obligations for the reviewer. By blocking the PR from progressing, they have taken an ownership stake in it and should make themselves available to answer clarifying questions. They should also give clear feedback on what needs to change for the PR to get approved."
Leaving any review usually comes with these obligations. I would prefer to see this lifted into some general section. I do think we can highlight that request changes blocks the PR from moving.
There was a problem hiding this comment.
I tried to bridge this distinction with the changes I just pushed - I added a section at the top of the "Reviewing" section that applies to all types of review, and changed this section to indicate that "additional responsibility" is implied by blocking the PR. Let me know how that looks.
| Once a community PRs has been created, they will be automatically be linked to an internal Jira ticket. The | ||
| internal ticket is used for prioritization and tracking purposes. |
There was a problem hiding this comment.
The reason this was inlined before was to get it in the same sentence. Just wanted to verify this is an intentional change.
There was a problem hiding this comment.
I think it's back to the way it was now. I had separated it out so that the tags were clearer, but I do see that it flows better as a single paragraph.
|
|
||
| </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.
| - 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. |
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.
| While we mostly use an asynchronous review process, please don't hesitate to schedule a meeting with | ||
| the author to discuss the changes. While asynchronous communication can be useful, it incurs a time | ||
| penalty which can drag out the review process. Sometimes setting up a short call to discuss the | ||
| changes can potentially save a lot of time. |
There was a problem hiding this comment.
| changes can potentially save a lot of time. | |
| changes can save a lot of time. |
There was a problem hiding this comment.
This has been fixed with the latest set of commits.
| We shouldn’t hesitate to use this status. However, it does come with obligations for the reviewer. | ||
| By blocking the PR from progressing, they have taken an ownership stake in it and should make | ||
| themselves available to answer clarifying questions. They should also give clear feedback on what | ||
| needs to change for the PR to get approved. Likewise, a PR author should not be discouraged by a | ||
| request for changes, it's simply an indication that changes should be made prior to the PR being | ||
| merged. This is common. |
There was a problem hiding this comment.
Can you elaborate? Not sure why this language change does anything more than expect the reviewer to stay engaged.
I too want to be sure "request changes" is taken very objectively.
| @@ -113,6 +140,7 @@ a time. | |||
| - Does the PR change the areas you expect to be changed? | |||
| - Are any missing? | |||
| - Are any present you didn't expect? | |||
| - Are there unit tests present? | |||
There was a problem hiding this comment.
| - Are there unit tests present? | |
| - Are unit tests present? |
There was a problem hiding this comment.
Fixed in the latest commits.
|
|
||
| <community> | ||
| Once a community PRs has been created, they will be automatically be linked to an internal Jira ticket. The |
There was a problem hiding this comment.
| Once a community PRs has been created, they will be automatically be linked to an internal Jira ticket. The | |
| Once a community PR has been created, it will be automatically be linked to an internal Jira ticket. The |
There was a problem hiding this comment.
Fixed in the latest commits.
| - 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. |
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?
| 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 **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.
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.
| :::info 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. |
There was a problem hiding this comment.
| team(s) - or to individual engineers if assigned - via Slack to follow up. | |
| team(s) -- or to individual engineers if assigned -- via Slack to follow up. |
There was a problem hiding this comment.
Fixed in the latest commits.
| 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 |
There was a problem hiding this comment.
ℹ️ Other thread will potentially need updating here too.
|
|
||
| </bitwarden> | ||
|
|
||
| <community> | ||
|
|
||
| Once a Community PR has been created, a Bitwarden developer will perform a code review. While we try |
There was a problem hiding this comment.
| Once a Community PR has been created, a Bitwarden developer will perform a code review. While we try | |
| Once a Community PR has been created a Bitwarden developer will perform a code review. While we try |
There was a problem hiding this comment.
Fixed in the latest commits.
|
Thank you @Hinton, @eliykat, and @withinfocus for the feedback! I've made some adjustments to the more trivial changes, but I feel there are a few larger outstanding items:
Without objections, I can raise these 3 questions to the larger group via Slack. |
There was a problem hiding this comment.
Is the preview working for others? It seems to not have these changes for me. On the open topics:
- Could we suggest "2-3 business days"? Feels like a middle ground for sometime in a business week to keep progress going.
- I feel they are and never had anything more than a Slack integration and email where it worked for much larger organizations.
- I suggest we leave it out. I feel there's a lot of disagreement over how much to prescribe on PR comments.
My remaining comments are super-minor.
| 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 ...
| - 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.
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.
| - Ask the author to split the work into a smaller PR for review | |
| - Ask the author to split the work into a smaller PR for review. Suggest reasonable axes to on which to divide the work (see [branching for large features](#link-to-new-section) |
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 master, I felt that it was better to focus on how to build small, incremental changes and not detail the complex branching scenarios we had to handle previously around Short-Lived Feature Branches and Long-Lived Feature Branches. I'd welcome any feedback on it, especially on ways to think about breaking up work - that was very much my attempt at condensing feedback and adding intuition to it to come up with some ideas.
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!
Co-authored-by: Matt Bishop <matt@withinfocus.com>
Co-authored-by: Matt Bishop <matt@withinfocus.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
Co-authored-by: Matt Bishop <matt@withinfocus.com>
| Choose **branching and merging into a long-lived feature branch** if the feature will: | ||
|
|
||
| - Require a single Pull Request to produce a testable, releasable change, or | ||
| - It is possible to put the changes behind a feature flag while multiple PRs are in flight. | ||
| - Require multiple pull requests to produce a testable, releasable change, **and** | ||
| - It is not possible to put the changes behind a feature flag |
There was a problem hiding this comment.
(non-blocking) Are we practicing long-lived feature branches at all any more? Is there any continued justification for them? I prefer to just have one way to do things (that is, the correct way) when possible.
There was a problem hiding this comment.
I feel like there is still a scenario in which large features would like to be introduced together due to either technical or product-driven considerations, but we can't put them behind a feature flag. The Vertical Vault Nav changes are an example. Perhaps I can supplement this with a follow-up PR that extracts this into a callout that highlights how it's not our current best practice?
| - You have verified that all possible changes have been | ||
| [unit tested](./../testing/unit/naming-conventions.mdx). |
There was a problem hiding this comment.
This is a major change to our current practices, which are less strict about this. This is worth highlighting when this is shared more broadly.
| 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. |
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
| - 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. |
There was a problem hiding this comment.
I expect that removing tech-leads as the default reviewer should help with notification overload.
|
|
||
| - 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 **48 hours** to: |
There was a problem hiding this comment.
I know this is going to be shared for discussion, but my 2 cents: I would prefer this be expressed in business days rather than hours. e.g. you cannot (should not) review code on public holidays or weekends.
2 business days seems fine to me as a starting point.
There was a problem hiding this comment.
This has been addressed with 97745a9 (#220).
|
@trmartin4 are you still working on this improvement? |
|
@MGibson1 yes, actually I had this in my To Do list and it's been surpassed by some other things that have come up. My next steps were to consolidate all the items that require highlighting as a significant change to our current process, and publish this change along with a notification in Slack. My current list of those updates are:
Am I missing anything else? |
@withinfocus these should already have been fixed in |
Objective
With the addition of
CODEOWNERSand the maturation of the component teams, I felt it would be good to clarify the expectations around PR authors and reviewers.These updates include some changes to the contract between PR author and reviewers, namely with the expectation that PR reviews should take place with 24 hours and that Github notifications should be the primary source of review requests. These have come from a running list that I've been compiling with pain points in our current process, gathered from retros and from other meetings.
I'd welcome any feedback and clarifications on this, as I'd like this PR to serve as a prompt for discussion prior to implementing any of these process changes. If agreed upon, I plan to update the teams in Slack as well to clarify and answer any questions.