Recently I read a tweet from @GergelyOrosz:
There are few engineering practices that I've not tried but I really want to. Post-commit reviews are one of them and @copyconstruct has a beyond excellent writeup. I'm thinking now is how to get to the stage that it's safe enough to fly with this:
He links to Cindy Sridharan's post on Medium about post-commit reviews which in it's opening paragraphs references a paper by Amazon on how they automate safe, hands-off deployments.
Then, after some preamble there's a section on what post-commit reviews are, and why you should prefer them over the more prevalent pre-commit reviews.
Now, let me be clear here, the 'commit' in question isn't a commit to a feature branch, or a PR, it's the merge commit that merges your work to trunk, or whatever you're now calling "the branch formerly referred to as 'master'".
So again, to be clear, we're talking about reviewing work post-PR merge, but before deploy - specifically that commits that have not been approved don't get deployed. So from now on to avoid confusion I'm going to call these reviews 'post-merge reviews'.
Now, at this point, I'm interestingly reading along, and wondering how Amazon makes this work, because I can foresee some problems with scale - and compliance, and security, and... - that would make a large company like Amazon pretty wary of doing something like this. I find nothing referencing Amazon's particular mitigation against the threats of this particular process of working, so I go back to the source reference and read the long, detailed Amazon paper by Clare Liguori on how CI/CD works at Amazon. It's full of lots of interesting detail, automated aggregate metrics that monitor phased deploys and automatically roll-back problematic deploys, pipelines as code, multiple pipelines for micro-services that are separated by concern (code, infrastructure, config, etc.) and lots of other good stuff.
There is no mention of post-merge reviews.
I'll let this settle in.
There is no mention of post-merge reviews.
I went back and read the Medium post again. Yup, maybe I'm just being dumb, but now that I'm actually looking for it the author's line is actually: "with all this cool stuff going on at Amazon, it's surprising that they're not doing this awesome thing that I thought up!".
OK. Whether quoting Amazon white papers that have nothing to do with the body of your post in your header is disingenuous or not I'll leave up to you. Just for reference, there is plenty of good research that proves the adjacency effect, e.g. this.
Let's now look at the proposed pros and cons of post-merge reviews according to the author:
- Focus - allows devs to merge PRs and continue on to future work without pausing for review comments/sign-off;
- Encourages better development practices - author argues this requires consensus, collaboration and automation. There doesn't seem to be anything about post-merge review that enables this, just a blanket "this works better if you have X, Y and Z", which although true isn't something unique brought to the table by post-merge reviews. Arguably most everything in dev works better with X, Y and Z;
- Post-merge reviews can detect more bugs before code review - this is not the review, per se, but actually the live environment testing pre-deploy. Author does make the point that there's no reason this can't be done with pre-merge PRs too, so again actually not a pro of this proposed process.
- High-functioning, high-trust environments - this is at best a partial constraint, not a con of the process, so not really a con in the same way that 2) above isn't really a pro.
- Investment in automation - I'm not sure that there is any more automation necessary for doing a post-merge review than doing a pre-merge review - both are possible with the same level, so also not really a con.
So in all we have one real pro left that is different about this process, namely reviewing post- rather than pre-merge.
It's worthwhile mentioning that although the author talks about the possibility of review post-deployment, they then spend the rest of the article talking about pre-deployment, post-merge reviews.
Now let's look at a couple of things that the author doesn't talk about:
- Transactionality - commit both or commit nothing
If I merge two commits, and one of them gets approved but the other doesn't, then what? Because there is no container of transactionality we'll have some of the functionality deployed but others undeployed. This is an obvious source of errors.
- Ordering - out of order commits introduce unwanted functionality (bugs)
If I merge some code, and you branch from that point for your work. My code has yet to be approved, but what happens to your code? Is your code now dependant on my code's PR review? Does a successfully PR review of your code auto-deploy mine? This introduces all kinds of potential error scenarios, plus it makes reviewing PRs more complex because we now need to know the status of all previous merged code PRs...
Given the dubious benefits of a post-merge pre-deploy review process, and the obvious (and not-so obvious) downsides, I cannot recommend the practice. On the other hand I can, in certain circumstances, recommend post-deploy reviews.
When working with FutureLearn, I observed them working with post-deploy reviews, which seemed a bit backwards when I was there, but quickly struck me as a good idea, for their particular situation. They were a Ruby shop, with a tech team of less than fifty people, building a eLearning platform as a typical Rails-style monolith. There was 100% or near-enough test coverage on all things. Almost all work was paired-on, and if it was then a PR was opened for review post-merge, post-deploy. And changes required by the review would be rolled forward. Only code that had not been paired on, or it was felt needed broader exposure mandated a pre-merge PR review.
I would posit that if your situation was similar then post-deploy reviews may work for you.
Cindy Sridharan's post on Medium about post-commit reviews. Since the link above just links back to the gist.