I recently read an excellent article in the Amazon Builder’s Library by Clare Liguori which goes into great detail about AWS’s CI/CD architecture. It’s a truly brilliant post and I recommend everyone interested in CI/CD infrastructures read the article. The article covers the gamut from unit testing to integration testing (in production) to staged rollouts in “waves” to automated rollbacks. I guarantee there’s something valuable you’ll learn from the article even if you’re not building the largest distributed systems in the world like AWS does.

The only surprising thing to me about the article was that it stated code review was mandatory before merging a change to the main branch.

All changes going to production start with a code review and must be approved by a team member before merging into the mainline branch (our version of “main” or “trunk”), which automatically starts the pipeline. The pipeline enforces the requirement that all commits on the mainline branch must be code reviewed and approved by a member of the service team for that pipeline. The pipeline will block any unreviewed commits from being deployed.

With fully automated pipelines, the code review is the last manual review and approval that a code change receives from an engineer before being deployed to production, so this is a critical step. Code reviewers evaluate the code’s correctness and also evaluate whether the change can be safely deployed to production. They evaluate whether the code has sufficient tests (unit tests, integration tests, and canary tests), whether it is sufficiently instrumented for deployment monitoring, and whether it can be safely rolled back.

Image for post
Image for post
Pre-Commit Code Review Workflows

While mandatory pre-commit code review is generally not uncommon at many organizations (as a matter of fact, it’s pretty much the norm for many open source projects and organizations), the reason I was surprised was because I’d assumed the extremely robust testing environments and infrastructure detailed in the post would’ve afforded more latitude when it came to merging code to the main branch.

Code Reviews and Developer Velocity

Our results show that, although the top motivation driving code reviews is still finding defects, the practice and the actual outcomes are less about finding errors than expected: Defect related comments comprise a small proportion and mainly cover small logical low-level issues. At the same time, code review additionally provides a wide spectrum of benefits to software teams, such as knowledge transfer, team awareness, and improved solutions to problems. Moreover, we found that context and change understanding is the key of any review.

While there are studies that estimate effective code review can detect about 90% of bugs, mandatory pre-commit code review hinders developer velocity almost invariably.

In the best case scenario, a pull request can be merged to mainline in a matter of seconds after it’s submitted. This requires that:

  • CI run all tests of a project in a matter of seconds
  • turnaround time for code review is in the order of a few seconds

But more commonly, both the CI and the code review phase take minutes to hours to complete, requiring the developer to wait that long to merge their pull request. This is especially true for larger feature branches, though sometimes, even a relatively small patch can get bogged down for days in code review for a number of different reasons (the code owner might be on vacation, the project in question might be a completely volunteer driven open source project, there might be a lot of back and forth between the reviewer and the reviewee on the change and so forth).

On average, I’d argue a code review slows a developer down from a few minutes to a few hours, and in the worst case scenario, a few days or even months (when a change results in a long-running pull-request requiring the developer to rebase off of the main branch multiple times to avoid merge conflicts).

Post-Commit Reviews

In many ways, post-commit reviews offer the best of both worlds: developer velocity is not sacrificed at the altar of waiting for an approval, and reasonable concerns get addressed in short order by the developer in follow-up commits.

While there are a number of caveats to post-commit reviews (detailed in the following sections of this article), preponderantly favoring post-commit reviews can allow developers to rapidly iterate on features they’re developing while also keeping their changes small.

Post-Commit Reviews != Post-Deployment Reviews

Image for post
Image for post
Post-Commit Pre-Deploy Reviews

Post-commit reviews offer a lot more flexibility than people imagine. “Post-commit” review can mean:

  • leaving code-review to be the very last step of the deployment pipeline, before the code is deployed to production
  • or in cases where it’s not necessary for an audit-trail or mandatory sign-off on changes (for prototype projects, internal tools that don’t handle customer sensitive data and so forth), for code review to happen post-deployment.

In both of the aforementioned scenarios, post-commit reviews offers plenty of benefits.

Benefits of Post-Commit Reviews

Focus

Being able to merge pull requests in quick succession ensures that the developer can iterate faster on the feature they are developing. It also encourages the developer to keep pull requests small, ensuring a feature is developed in many small pull requests (or at the very least comprising of many atomic commits).

Another benefit of post-commit reviews is reviewer focus. In a culture where pull requests are small and numerous, being pinged every 10 or 15 minutes for a review can hinder the productivity of both the reviewer and the reviewee.

Moving to a post-commit review format frees the developer from requiring approval for every pull request. It also allows the reviewer to batch reviews, so they are reviewing a number of changes in one fell swoop, as opposed to being constantly interrupted for code reviews. Furthermore, this incentivizes reviewers to only comment on merged pull requests that raise concerns or have logical bugs. In my own experience, this reduces nitpicking on the reviewer’s part, and in the event the reviewer does nitpick, it allows the developer to address the minor concerns at their leisure.

Encourages Better Development Practices

Getting to a point where post-commit reviews are a reality requires a strong cultural scaffolding. At the very least, it requires:

  • a culture of collaborating on aspects like API design prior to code implementation via a design document.
  • consensus around aspects like style-guide, coding idioms, concurrency primitives etc.
  • investment in better automation practices and tooling.

If a design document has been clearly fleshed out and agreed upon before the code is implemented, the implementation will not comprise of any major surprises to the reviewer. This in turn minimizes (or even eliminates) the need for reverting commits due to API design or implementation specific disagreements between the reviewer and the reviewee.

For post-commit reviews to be truly effective, it’s also important to minimize code review comments pertaining to code style or idioms. This eliminates the need for the reviewer to comment on the automatable aspects, as it were, of code review.

One can argue that all these are the hallmarks of a good development culture anyway (and for many projects, these are already implemented as a part of the pre-commit CI workflow). That post-commit review makes them inevitable only acts as a forcing function to foster better development practices.

Post-Commit Reviews Can Detect More Bugs Before Code Review

The reason why “post-commit” tests are required is because unit-tests (while incredibly effective at preventing many forms of critical failures), lint tests and the like aren’t sufficient to detect certain categories of bugs that only surface in real environments when testing against real dependencies.

By “post-commit” testing, I’m referring to all the tests a code change is subjected to post-commit but pre-deploy: integration tests, load tests, soak tests, shadow tests, exception tracking and so forth.

There’s no reason why most of the “post-commit” testing needs to happen after code-review. There’s a stronger likelihood that sufficiently sophisticated “post-commit” testing can detect more bugs and alert the developer of the bug (or even better, automatically revert the commit in source control), allowing the developer who wrote the code to understand the nature of the bug and fix it, before a reviewer has even looked at the code.

This means that the reviewer can have a fair degree of confidence that the change is functionally correct and has undergone extensive testing before they review it. The reviewer can then truly only focus on trying to find the categories of bugs that even the most advanced forms of “post-commit” testing doesn’t flag: security bugs, a bug in the implementation of business logic not caught by any of the tests and so forth.

There might even be scenarios where it’s not necessary for a code reviewer to sign-off on a change before it’s deployed to production. This is true for most prototype projects, projects without compliance requirements, projects that don’t handle customer data or sensitive data and so forth. In such cases, a “post-commit” code review can actually even be a “post-deploy” code review, where code is only reviewed once it’s been deployed to production. A concrete example of a company that practices post-commit, post-deploy reviews is Quora. This blog post details a lot more about Quora’s post-commit review approach.

Challenges of Post-Commit Reviews

High-Functioning, High-Trust Environments

In such cases, it might help to begin onboarding a new team-member by starting with “pre-commit” reviews and work towards the goal of bringing the team member up-to-speed enough to gradually switch to doing post-commit reviews. Open-source projects can work in a “mixed-mode”, where maintainers of the project can rely on post-commit reviews while new contributors can be onboarded by offering them pre-commit reviews. Part of attaining maintainership of a project can involve progressing from requiring pre-commit reviews to requiring post-commit reviews.

Investment in Automation

This is arguably even harder to achieve in organizations that have embraced the monorepo, since reverting a single offending commit could involve having to revert tens or hundreds of commits across the codebase. Most organizations that have adopted a monorepo have extensive automation and tooling to this end. An example is how Uber keeps the main branch green at scale at all times using a system called SubmitQueue. SubmitQueue, in a nutshell, is a meta-build system, a load balancer and a speculative executor rolled into one, with a dose of data science thrown in for good measure, which guarantees “an always green master branch at scale” via serializability of commits to the main branch when there could be hundreds of concurrent commits.

In my own experience, managing commits becomes a lot easier in non-monorepo environments, since the automation required to revert commits doesn’t require research paper levels of complexity.

Conclusion

  • post-commit, pre-deploy reviews
  • post-commit, post-deploy reviews

As with all good things, it requires time and investment to get right, but for teams or organizations trying to improve the developer productivity, it’s certainly an avenue worth exploring.

Written by

@copyconstruct on Twitter. views expressed on this blog are solely mine, not those of present or past employers.

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store