Reduce cycle time with effective pull requests
Staff Software Engineer
A pull request (PR) is (quite literally) a request to pull a change into a project’s code or documentation. It is a popular change management process supported by many VCS providers including GitHub, GitLab, Bitbucket, Codeberg, and others. Typically these come with features to track open pull requests, tools to assist in reviewing the changes, the ability to approve—or reject—PRs, and finally to merge approved PRs.
Teams that adopt continuous deployment (CD) usually want an automated process where merging a PR automatically kicks off the deployment of a new version of the service to production. This is a case where having an effective review process is crucial.
If it’s not clear how to validate a change, or even why the change might be needed, reviewers may experience fear, uncertainty, and doubt (FUD). FUD can cause reviewers to leave requests for clarifications, requiring even more reviews—even if the changes to the code look benign. These additional reviews increase cycle time, increase the number of context switches required by reviewers, and decrease velocity.
A new feature or bug fix cannot reach users until it has passed review. The good news is we can reduce the likelihood of additional reviews affecting cycle time by making our PRs more effective.
Use a pull request template
Reviewing a code change taken at face value is easy. It passes tests, it doesn’t trigger the linter, doesn’t have quadratic algorithms, etc. In complex code bases, especially big or legacy ones, the stumbling point is often the why of a change, not the code itself. How can you verify that it works as intended when hitting production?
If you use a PR template, you can ensure pull requests have a common structure. You can prompt for the information you want available when reviewing pull requests. Pull request templates are supported by both GitHub and GitLab. To create your own use these sections:
Background
Give enough context for reviewers to understand why the change is being made. Explain your choice of implementation, especially if it’s not obvious. Are there edge cases that ruled out a simpler implementation? Performance concerns? How does the new code deal with the legacy state if the shape of the new state changes?
If the change is best reviewed commit-by-commit, note it here. Also note if it is best to ignore whitespace changes during the review. Also note if it is best to ignore whitespace changes during the review.
Proposed changes
- A bulleted list of notable changes.
- Provide examples, if possible.
Deployment considerations
Include enough context that someone else could manage the deployment.
- Should it be deployed at a particular time of day, such as during low-traffic periods?
- Are there customer-visible changes that need to be communicated before this can be deployed?
- Will this change affect other teams? Are they aware?
- Will you use a canary deployment strategy to roll out the change before deploying to a larger user group? If not, why not?
- Do you need to activate a feature flag after deployment? Or a roll-out flag?
Validation
Provide explicit steps for validating the change in a canary rollout or production.
- Which metrics should be used?
- Are there helpful links to notes and other information?
- If you’re using a roll-out flag, what are the steps to implement it? How long do you let it “cook” at each step?
Rollback
How do you roll back quickly if there are unforeseen problems with the deployment?
Leave the clarifying text in the PR template to guide the person filling in the PR. Adapt these recommendations to your particular situation: If you do not use canary deployments, don’t use that as an example. If you always use Helm to rollback a deployment, add helm rollback my-service-name in the template to save time. A good alternative is a link to a rollback procedure.
Note: You might also want to add an instruction to delete the example text so reviewers don’t have to skim past it in every PR.
Tips for minimizing PR cycle times
This section is aimed at the person opening the pull request, to ensure that they get approved with a minimum of cycle times. Let’s assume that their code is perfect, and only focus on improving the PR description.
Give detailed background on why the change was made
Link to the planning/ticketing tool you use. But don’t force people to follow the link to get the info they need, because they likely won’t.
Give explicit steps to perform validation
How do we know that this change behaves as it should? Repeat for canary stage and after deployment to production.
When considering how to validate your change in production you may need to go back and rework the change. Don’t be afraid to add new metrics or log statements purely for the purpose of validating the change! You can always remove them later. Create a custom Datadog notebook, if there isn’t a relevant dashboard handy, and link to it.
Learning to think about this up front while implementing the code in the first place means you will reduce your personal cycle time, too.
Consider whether to use a roll-out style feature flag
These let you roll out the feature in a limited way, for example to 1%, 10%, or 50% of traffic. Equally important: It allows you to back out the impact immediately, without rolling back the code.
Feature flags are useful for managing risk, but may be overkill for all changes as it requires effort and discipline to clean up obsolete ones. Not doing so can get expensive very quickly.
Split the change into several commits to ease review
Always try to keep a clean commit history. Split a single commit into several if it improves the reviewer’s experience. This is often the case if there are several distinct refactorings touching the same code.
Ignore changes to whitespace
Note when changes to whitespace should be ignored. You can even include a link to the diff with ?w=1 added to the URL. It’s not always obvious if you’re coming at the PR “cold”, and inexperienced reviewers might not even be aware that this is an option.
This can reduce the burden on reviewers if the code change changes the level of indentation in existing code, for example by wrapping it in an if/while/for control structure.
Putting it all together
Here are two example PRs, one that’s of poor quality and another that’s detailed and helpful.
What not to do
Title: Changes to Home Page
Changes:
- Did some work on the images on the home page.
Deployment:
- Just merge and deploy whenever. It should be fine.
Validation:
- Check if the images load faster on the home page.
Rollback:
- Not sure. Revert the changes?
This PR description lacks proper context, does not detail the proposed changes, does not have sufficient instructions for validation, and does not provide a clear rollback plan. It will likely lead to increased cycle times as reviewers ask for clarification.
Good PR example
Title: Optimize Image Loading for Home Page
Background:
We have been facing page load issues on the home page due to high-resolution images being loaded. This PR aims to fix this by implementing lazy loading of images. The new approach uses the Intersection Observer API to load images as they come into the viewport. This should enhance our page load times and improve the overall user experience.
Proposed Changes:
- Implemented lazy loading of images on home page using Intersection Observer API
- Added fallback for browsers that don’t support the Intersection Observer API
- Updated unit tests to accommodate new image loading strategy
Deployment Considerations:
- No specific timing for deployment; the changes should not impact user experience during high traffic periods
- No customer-visible changes that need communication
- No impact on other teams
- No feature flag activation or roll-out flag needed
Validation:
- On staging, verify the page load times have improved using the ChromeDevTools performance tab.
- On production, monitor network traffic and CPU usage to ensure improvements are evident.
Rollback:
In case of unforeseen issues, rollback to the previous version using helm rollback image-service-name.
This PR provides enough information for a reviewer to understand what changes were made, why they were made, how to roll them out successfully, and how to respond to any issues in production.
Conclusion
As a service owner, using a PR template to prompt for relevant information makes it easier for reviewers to get the context they need to review the PR in one sitting, and reducing context switches and cycle time.
As an author, spending extra effort filling in the PR template (use your own if the project doesn’t have one) may take longer to get to first review. But by reducing cycle time your PR should spend less time in review overall. And thinking through deployment concerns, how the code will work with legacy data, and how to validate the change, will often have a positive impact on your code.
Optimizing for getting to “approved” with the minimum of cycle time, rather than bouncing PRs back and forth between author and reviewers means more time for planned, creative, work, and less time spent reacting to issues.
