Stop! You’ve been using pull requests all wrong
Practical tips for making pull requests 1000% more useful
Most succesful processes use peer reviews to ensure quality. Software development is no different. It is common that code changes must be reviewed in a pull request before it is merged to mainline branch.
Over the years, I’ve submitted a few pull requests, and reviewed a ton of them. During this time, I’ve discovered tools and processes that make pull requests much more effective, for both those submitting and those reviewing them.
Stop arguing over preferences
This ensures files are formatted the same, so pull requests will only contain source changes (as opposed to showing large change set because my ide changed linux line endings to windows ones). This will also free up time as reviewers won’t argue about inconsequential changes, such as whether brackets go on the same line as an if statement or the next.
Automatically reject PRs that don’t meet standards
- Add a static code analysis tool to your CI process. Have it fail the build if rules are breached
Static code analysis provides a consistent, automated way to ensure code meets certain standards (and you can adapt these standards on a team by team basis). For example, this can make sure there are no methods longer than 50 lines, or enforce 80% unit test coverage. If a pull request does not meet the standards, it fails the build. These checks can be ran before even requesting review from someone else.
Focus on recoverability/mitigation before code correctness
Reviewing code is an error-prone process. Even the best reviewers will make mistakes. Given the inevitability of errors (especially unknown ones), when reviewing a pr, we should focus most of our attention on the processes used to minimize the customer impact of these errors. In practice, this usually means doing the following
- Ensure code can be safely rolled back
- Ensure usage of canary or linear deployments (as opposed to all at once)
- Ensure processing is asynchronous whenever possible
- Ensure failed events are persisted in dead letter queues
- Ensure code emits proper custom cloudwatch metrics (likely using EMF)
- Ensure cloudwatch alarms on errors and latency
- Ensure automatic rollback on relevant alarms
- Ensure operational procedures (runbooks) for reprocessing failed items
Make PRs smaller
- Change sets (pr) should be tiny (could be 20 lines)
- Try to rebase/merge at least twice a day
By making frequent, small, reversible changes, we can determine exactly what change lead to increased errors/latency. We have used the techniques defined above to ensure minimal customer impact, so now all we need to worry about is restoring the mainline branch to a working state. This can be done with one command
git revert <commitHashHere>
Pushing smaller change sets also makes reviewing pull requests easier, and developers will be more likely to review them thoroughly.
smaller pull requests also minimizes the risks of errors being introduced (more code = more bugs)
This also avoids merge hell, as the number of files changed at once will be much smaller.
Trunk-based development anyone?
As I’ve started creating smaller pull requests more and more frequently, to the point where pull requests are essentially a single commit, I found myself asking, “what is the point of these pull requests?”. I’ve tried true trunk-based development with a few teams, and have found it effective with proper automation.
Conclusion
There you’ve got it; my advice on how to produce higher quality solutions by improving the tools, processes, and techniques surrounding pull requests. Please do let me know in the comments if you have any techniques that I haven’t described here!
