Read on Twitter
One thing we do at Wave that I really like is *post-deploy* code review.
“Wat?!” Yep! Once you’re done coding, type “bors merge” and 15 minutes later your code is in prod. *Then* it gets randomly assigned to a teammate to review and a Slack bot bugs them until they’ve done so.
“Wat?!” Yep! Once you’re done coding, type “bors merge” and 15 minutes later your code is in prod. *Then* it gets randomly assigned to a teammate to review and a Slack bot bugs them until they’ve done so.
This is great for many reasons:
• As an author, you don’t get blocked on review and can iterate much more quickly
• As a reviewer, you don’t have to break flow for urgent code reviews, you can batch them to a time that’s convenient
• And you have time to be more thorough!
• As an author, you don’t get blocked on review and can iterate much more quickly
• As a reviewer, you don’t have to break flow for urgent code reviews, you can batch them to a time that’s convenient
• And you have time to be more thorough!
“But don’t you ship bugs all the time?!”
When I notice a bug in code review, it’s usually because I thought “that looks funny” and then *executed a bunch of code in my head* to verify.
You know who’s bad at executing code? Humans. You know who’s good at it?
computers
When I notice a bug in code review, it’s usually because I thought “that looks funny” and then *executed a bunch of code in my head* to verify.
You know who’s bad at executing code? Humans. You know who’s good at it?
computers
So it shouldn’t be the reviewer’s job to find bugs. It should be your tests! And if people know the tests are what stands between them and prod bugs, they write decent tests :)
(PS: The right reaction to “that looks funny” is actually: “Is there a test?” If not, ask for one!)
(PS: The right reaction to “that looks funny” is actually: “Is there a test?” If not, ask for one!)
“But what if someone ships a bad DB schema / mobile API and it’s hard to reverse?”
Yeah, it sucks when that happens! It also sucks when you write a bunch of code with a bad DB schema / mobile API and then some code reviewer tells you to change it when you just wanna ship :)
Yeah, it sucks when that happens! It also sucks when you write a bunch of code with a bad DB schema / mobile API and then some code reviewer tells you to change it when you just wanna ship :)
We solve this with *design review*—clear your design with someone else *before writing (very much) code.* If that review works well, you rarely end up having to redo stuff in prod.
(Of course, sometimes things get missed in design review. But that’s true of code review too!)
(Of course, sometimes things get missed in design review. But that’s true of code review too!)
“What about security?”
What about it? We got ISO 27001 certified with this process :)
Security is about evaluating and trading off risks, not setting all risks to 0. If you can lower risk enough by protecting ability to push + doing post-deploy reviews quickly, it’s fine.
What about it? We got ISO 27001 certified with this process :)
Security is about evaluating and trading off risks, not setting all risks to 0. If you can lower risk enough by protecting ability to push + doing post-deploy reviews quickly, it’s fine.
“But don’t people procrastinate on reviews if they’re not blocking?”
Yes—this is the biggest downside, and we’re still working on making it work well. We do the majority of our code reviews quickly, but some languish. (Especially mine
until I took myself out of rotation!)
Yes—this is the biggest downside, and we’re still working on making it work well. We do the majority of our code reviews quickly, but some languish. (Especially mine
until I took myself out of rotation!)
But... is “let’s block everything!!” really the right reaction to that? I don’t think so. It’s infantilizing to conclude that the only way to get reviewers to do a good enough job is to punish both them *and other people* by blocking shipping. I think there’s a better way!
PS: organizational context relevant to why this works well for us:
• ~30 eng divided into 6 smaller teams
• Distributed across 9 timezones
• Main artifact is a ~100kloc, well-tested, statically typed, continuously deployed monolith
As always, YMMV in other configurations :)
• ~30 eng divided into 6 smaller teams
• Distributed across 9 timezones
• Main artifact is a ~100kloc, well-tested, statically typed, continuously deployed monolith
As always, YMMV in other configurations :)
