Ship / Show / Ask: A modern branching strategy(martinfowler.com)
martinfowler.com
Ship / Show / Ask: A modern branching strategy
https://martinfowler.com/articles/ship-show-ask.html
150 comments
Yeah the quote you pulled is a really unhealthy take. Requiring reviews is not lacking trust in the dev team, it's acknowledging that we are human, fallible, and that these systems are full of complexity that one person seldom understands on their own.
If you view a code review as the reviewer seeing you as untrustworthy, you are likely some combination of sensitive and arrogant.
If you view a code review as the reviewer seeing you as untrustworthy, you are likely some combination of sensitive and arrogant.
All my developers like the fact that someone else looks at their code. It distributes the responsibility, facilitate knowledge transfer, puts emphasis on readability, ensures that plumbing and "boring" code is idiomatic and plainly result in less bugs.
Although I do agree with all of this. I sometimes feel like "distributes the responsibility" can actually cause things to slip through the cracks. The reviewer thinks "eh, I didn't write this, what I saw looked fine" and the author thinks "hey they approved it, so I'm not out on a limb here", causing both sides to look at the code less closely.
I'm not saying that always happens, but it does sometimes in my experience.
I'm not saying that always happens, but it does sometimes in my experience.
One team I used to work on had a really healthy way of dealing with this. Whenever a bug made it through to production, the team would have an informal post-mortem meeting where everyone who worked on the change in question would talk about the cause of the bug, and perhaps try to identify ways something like that could have been avoided.
There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.
There was no real interest in assigning blame to individuals. I think that was largely because we required two reviewers to approve every code review. So, when something did happen, it had to make it past at least three people. That automatically makes the mistake a team responsibility and not an individual one.
As a developer, I agree. With perhaps, like the sibling from ’city41, the responsibility. I wrote it, it’s my code. Maybe QA can have their share if responsibility, if any later issue is something QA could have found.
But it requires an environment where responsibility doesn’t equal blame.
This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.
But it requires an environment where responsibility doesn’t equal blame.
This is also one additional reason why I don’t touch Github Copilot. Because I didn’t write that code.
Agreed absolutely.
This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.
This is also the sole reason I like sprints. They have storypointing, and that leads to shared understanding and better context switching throughout the sprint.
Yes! Too bad people rarely bother, nobody seems to have the time.
if code reviews come with responsibility sharing then the are going to slow things further, 100% agree with the rest.
How about dealing with "that guy" who will take you through 8 revisions until he basically wrote the PR?
I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".
I typically just identify and avoid, get reviews from other people instead.. would love to hear more advanced strats. Attempting to talk about it never works because they dig their heels in on "I'm just enforcing quality".
This is a challenging balance to strike. I used to work with someone who I considered "that guy", but there was no denying that the code ended up better for it, even if it would have been basically fine otherwise. Especially over the long term, those little things can add up.
On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.
On the reviewer side, I have sometimes been in the situation where I end up letting something slide because I didn't want to be "that guy". Then I end up reading the code later and needing to fix it.
I think the answer here unfortunately relates to power dynamics. I'm senior, and others believe I'm senior, so when I dismiss reviews like this, no one fights me. Sometimes, with folks like this, even without being senior, just standing up to them works.
As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").
As others have said, using linters, auto-formatters, etc often helps with this as a lot of the things these people like to nitpick are then standards enforced by code, and you can send them off to fight people about the standard ("but not in this PR").
Two things, first have a standard to compare to. Second, use code linters and formatters to automatically enforce it.
That obviates trivial back and forth, leaving the interesting issues.
That obviates trivial back and forth, leaving the interesting issues.
You'd be surprised. Often that dude will have preferences that go way beyond linters and may even contradict widely accepted best practices.
I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..
My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.
I've worked with the dude who insisted on factoring out functions for unrelated 2-liners, creating extra coupling where we don't want it, the dude who insisted on mocking library code in tests when it would be easier and more thorough to just use it and make sure we're calling it right..
My favorite was the dude who, after 3 days of back and forth changes on a semi-urgent PR goes silent for a day, I ping him, he says "thinking of more comments :)" with the goddamned smiley.
Hah! The simple answer to that however, is "No." With a link to standards/best-practices if you want to be polite.
Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.
Also, sometimes other people are right... something to remind myself of. ;-)
Unless dude is your boss, in which the answer is "Yes," and the paycheck arrives on Friday.
Also, sometimes other people are right... something to remind myself of. ;-)
Yes, or the guy who wants you to mock out database calls because it's not a "real unit test" otherwise. So you spend another day mocking out crap when it's both easier and less error-prone to use the real thing. I guess the time spent will pay for itself... in 10 years.
The point here is to be able to run the tests without a database, and in this case they're probably right. Saying it's for the sake of purity of the test type is wrong, but you should mock out database calls in unit tests because you want them to run fast and without dependencies.
Move those tests to integration...
Move those tests to integration...
We don’t run tests without a database. Never been anywhere that did. I greatly prefer integration tests, in general. They almost always have value.
I dislike code reviews because there is always some guy deciding how things should be. Let's not pretend that the developer has any special say in how the code should look, even though he wrote it. It can be highly demotivating to work as a programmer in such teams.
Usually you have a few guys deciding everything.
Usually you have a few guys deciding everything.
90% of people are fine and code review has tons of benefits.. but the format is an enabler for 'that guy', as you're asking them for approval rather than running it by them as an FYI.
I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.
I've seen a few different motivated hard workers get to hard work on demanding as many changes as possible whenever they are on a CR.
There is also the flip side too where a block of code might just have a lot of problems and I’m trying to evaluate if it’s too hard in a public forum to flag all of them at once. If possible I try to flag a couple and to sit down and offer to do a one on one for more.
+1 to taking it offline when it's more than a couple minor style fixes.
I think it all comes down to judgement and there are definitely ways people can write code in their individual style without hurting maintainability.
That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache.
It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?
That said, maintaining a codebase where there is a lot of variation in idioms, style and formatting can be a big headache.
It adds cognitive overhead and if you're changing files you constantly have to balance preserving the local style with other factors. E.g. if someone likes aligning their equal signs, and you add a new line where the LHS is one character longer, do you reformat the whole block?
I can't fathom to be honest to be back in a place where something like that is even a question.
We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.
If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"
We use spotless. We use prettier. Nothing passes the build if you don't adhere to the style rules. You may disagree with some stuff. Make a case! Change the rule definition, apply them and show the PR to a representative cut of the developer community. You might get it through. We've done a few like that in my time here. Or you might realize what that rule change really means in some cases and the PR is ultimately declined. Has also happened. Ruled looked good for the place that triggered the want. Sucked in most other places, so we decided not to do it.
If you have a guy that can't live with not getting his will 100% of the time my advice is to try and get rid of him as soon as you can. Or run yourself. Not healthy in the long run and frankly "I'm too old for that shit. Now get off my lawn"
I think it’s the “requiring” a review part that comes across as not trustworthy. A review isn’t required on my team, but we do it most of the time. There have been plenty of trivial changes I’ve made where getting a review would have been a waste of everyone’s time or just been “rubber stamped” anyway. I just my team and my team trusts me to make that judgement call.
The problem is the subset of devs who don't know what they don't know.
Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it"
Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"
Dev 1: "I'm just going to reword this error message because I think it sounds awkward. Harmless, I'll ship it"
Dev 2: "Actually you've removed a keyword that our error messaging framework uses to highlight the error in neon red. We've seen in a/b testing that this bright color prevents error blindness 65% of the time, and saves users from having to fix mistakes that cost them on average 1 hour of confusion"
I thought you were going to say “now this string has to get retranslated and until then everyone will have a crappy experience” lol
Changing strings are about the most harmful thing to the bottom line where I work. :)
However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.
Changing strings are about the most harmful thing to the bottom line where I work. :)
However, in your case, that should probably be automated so people can’t do that just like we helpfully provide similar already translated strings when you add new strings or change them. People shouldn’t be able to break production, review or no.
Haha, absolutely, it's a terrible example with terrible design that I pulled out of thin air, but it was just to illustrate the point of how even seemingly innocuous changes can have major consequences.
And the original author's point is that you can't possibly happen to foresee all of the consequences, and so the best thing to do is to just ship the change and deal with what breaks.
That's the whole point of continuous integration, and that's what the author is trying to push. That you actually move faster by letting people push and fix bugs versus push a change, wait some time for a code review (somewhere 1min...1day), go back and forth on some style things, maybe a the reviewer managed to win the game of "spot the bug," and then ship, and then fix any delinquent bugs.
Like, unless if that middle reviewer is doing something that really can't be done elsewhere, it just serves to slow things down.
That's the whole point of continuous integration, and that's what the author is trying to push. That you actually move faster by letting people push and fix bugs versus push a change, wait some time for a code review (somewhere 1min...1day), go back and forth on some style things, maybe a the reviewer managed to win the game of "spot the bug," and then ship, and then fix any delinquent bugs.
Like, unless if that middle reviewer is doing something that really can't be done elsewhere, it just serves to slow things down.
> Actually you've removed a keyword that our error messaging framework uses
Hmm, so something is parsing a string looking for specific character sequences? I'd say the developer who changed that is doing the team a favor by surfacing a terrible practice.
Edit: NVM I see you acknowledged that in another comment.
Hmm, so something is parsing a string looking for specific character sequences? I'd say the developer who changed that is doing the team a favor by surfacing a terrible practice.
Edit: NVM I see you acknowledged that in another comment.
The reason for requiring a review on a team with high trust is that you can trust your team to validate the assumption that a given change is “trivial”. My team has had several minor changes recently where mistakes were caught in the review process.
If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.
If your team thinks it’s a waste of time, they’ll treat it as one. But that doesn’t mean it is one.
In many companies changes must be reviewed for compliance reasons (e.g., SOC2 will generally require some sort of review procedure).
> a really unhealthy take
And yet the attitude that developers must have something looking over their shoulders lest they ship code that doesn't meet some arbitrary metric is ubiquitous in the industry. Just look at all the automatic code checking incorporated into CI/CD pipelines.
Of course some of the automated checking is both healthy and reduces toil. Security sanity checks, for example, But take a look at your own organizations CI/CD pipeline and ask "how many of these checks are there because someone, somewhere, didn't trust the developers to do the right thing?"
This lack of trust in development teams is especially rampant in outsourced work. It's an attempt to paper over the fact that shipping your development to a contractor on the other side of the world is a terrible idea.
And yet the attitude that developers must have something looking over their shoulders lest they ship code that doesn't meet some arbitrary metric is ubiquitous in the industry. Just look at all the automatic code checking incorporated into CI/CD pipelines.
Of course some of the automated checking is both healthy and reduces toil. Security sanity checks, for example, But take a look at your own organizations CI/CD pipeline and ask "how many of these checks are there because someone, somewhere, didn't trust the developers to do the right thing?"
This lack of trust in development teams is especially rampant in outsourced work. It's an attempt to paper over the fact that shipping your development to a contractor on the other side of the world is a terrible idea.
Or you acknowledge that having a prettier and linter set up is a really awesome thing and you just make use of it.
At my place these things aren't there to tell you that you're a bad developer that doesn't care about well formatted and well written code.
They're tools that help you take out the drudgery of some of the formatting and style adherence. I am able to write code 80/20 clean and at the end I just run the prettier and linter to tell me about all the places I might have missed or 'not dealt with for now'. I even use out CI pipeline for this specifically because it runs the verifications faster than my local machine and I usually have other stuff to deal with anyway while I wait for that.
At my place these things aren't there to tell you that you're a bad developer that doesn't care about well formatted and well written code.
They're tools that help you take out the drudgery of some of the formatting and style adherence. I am able to write code 80/20 clean and at the end I just run the prettier and linter to tell me about all the places I might have missed or 'not dealt with for now'. I even use out CI pipeline for this specifically because it runs the verifications faster than my local machine and I usually have other stuff to deal with anyway while I wait for that.
But then for various reasons the prettier and linter become a tool to measure the difference between expectations and reality. Metrics are collected, performance indicators are established, and goals set. Pretty soon the tool is no longer helping the developers, but is instead judging and evaluating them, feeding results into stack-ranking and PIP systems.
Sorry to say this but then you are doing it wrong (not you you, your company - and I get it some are like that but not all). Our prettier and linter auto fix where possible. I literally just run them before I commit and let them auto fix. That's it. Done.
Some linter stuff the cmd line can't auto fix but IntelliJ can for some reason. Some I need to do myself. So what.
Sometimes I forget and the build complains. So what, I run, git commit --amend and force push. Done.
Since I'm a manager too I actually sometimes don't even amend and commit a 'aaah crap forgot prettier again' and push that so people see that it happens to all of us.
Caveat: we also squash our branches and each PR is exactly one commit on master (rebase merge strategy). Meaning your branch is yours to do with until its merge time. Nobody minds what you do. YMMV.
Some linter stuff the cmd line can't auto fix but IntelliJ can for some reason. Some I need to do myself. So what.
Sometimes I forget and the build complains. So what, I run, git commit --amend and force push. Done.
Since I'm a manager too I actually sometimes don't even amend and commit a 'aaah crap forgot prettier again' and push that so people see that it happens to all of us.
Caveat: we also squash our branches and each PR is exactly one commit on master (rebase merge strategy). Meaning your branch is yours to do with until its merge time. Nobody minds what you do. YMMV.
I've been labelled controversial (amongst other things..) in the past for stating this but hey ho, I'll take another leap:
Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly):
There is a difference between having reviews, and enforcing reviews.
In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people"
But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix"
Then comes the question of quality of review when they are mandated.
I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews)
Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants.
Code reviews are another tool in the box, but they should not be used for every job.
Aside from the "small changes, fast fixes" type of mantra, and "pairing is better than reviewing" that I suspect Martin Fowler is leaning on (and that I both support strongly):
There is a difference between having reviews, and enforcing reviews.
In my multi-decade career I'm yet to have a signle instance of the thought "thank god we prohibit changes that haven't been reviewed by two other people"
But I have lost count the number of times I've had the thought "I need to bypass this policy because shit's on fire and I've got a fix"
Then comes the question of quality of review when they are mandated.
I'll interject with: Code reviews are no inherently bad. Feedback is good. Collaboration is good. There are better methods of providing feedback than code reviews (I strongly object to the post-facto nature of code reviews)
Mandated peer reviews less so. Feedback is hurried, if not entirely absent, as it becomes another task that people _have_ to do. The time spent reviewing is rarely accounted for. There's a lot of knowledge transfer required for any non-trivial review to be effective, further increasing the demand on the participants.
Code reviews are another tool in the box, but they should not be used for every job.
My code reviews of others is often "please add a test", then nothing happens for 3 days, and I get people saying "when will this be done? it already passed UAT! get it moving". then stuff gets merged without tests, and the cycle repeats. Not ALWAYS, and commit/merge absent tests is sometimes required.
The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'.
Happy happy days...
The shoe was on the other foot a few years back and I was contracted on part of a large/legacy enterprise app product. I could 'fix' a bug, but couldn't always write a test, as I just didn't understand the system well enough to grok what was there. Policy was "you have to have a test". Fair enough. I remember hitting up 5-6 other devs on the team - some of whom had been there for 10 years - to pair for an hour or so to help me write a test. The same people who were blocking the PR requiring a test would not help me write a test. Some were honest enough to say "I don't know how you'd test that". But... it was frustrating. I had a 20 minute code patch (which demonstrably fixed the bug to the PM's satisfaction) take 8 calendar days to figure out how to test. Then a reviewer didn't like how the test setup was done. When they learned it was done with code from a colleague (and not of my own doing) they suddenly liked it and said it was 'clever'.
Happy happy days...
I think I left your previous work place a few month ago. It’s just too similar.
Each point.
was the company in the southeast US? :)
Contractors are generally paid by the hour. So while frustrating, there's always the paycheck to look forward to.
This is why having tests shouldn't be enforced by review, but if the code coverage drops for the PR, it fails tests.
> "I need to bypass this policy because shit's on fire and I've got a fix"
Want a list of catastrophes that occurred because someone did this?
Edit: Also, Fowler didn't write this article, it's just on his site.
Want a list of catastrophes that occurred because someone did this?
Edit: Also, Fowler didn't write this article, it's just on his site.
Want a list of absolutes that started as just examples?
I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.
I could write a long, long list of examples that were, in essence if not literally, typos that were missed by code review but bombed when in prod, too.
You mean like Mariner I? "The post-flight review also showed that a missing hyphen (technically a bar in the variable r̄) in coded computer instructions in the data-editing program allowed transmission of incorrect guidance signals." https://www.edn.com/mariner-1-destroyed-due-to-code-error-ju...
Or Arianne 5[0]
Both had code reviews. Both catastrophically failed.
No I am not trying to draw causation that code reviewed software ends in tragedy :)
[0]: https://www.bugsnag.com/blog/bug-day-ariane-5-disaster
Both had code reviews. Both catastrophically failed.
No I am not trying to draw causation that code reviewed software ends in tragedy :)
[0]: https://www.bugsnag.com/blog/bug-day-ariane-5-disaster
The Ariane software failure was slightly different because it only failed in the context of the differences in hardware between Ariane 1 and Ariane 5. It was perfectly valid code provided it was used within the constraints for which it was first designed. The Mariner code was simply wrong, and was destined to fail even under the conditions for which it was intended.
It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs.
I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.
It's a subtle difference, but I think an important one, especially when considering the complexities for program proofs.
I'm sort of (not eagerly) awaiting the day when something written in Rust is implicated in a catastrophic failure. All the platitudes about safety and guarantees will smack hard against a wall like Wile E. Coyote in a Road Runner flick.
I get the feeling that perhaps you haven't seen code review done well.
> lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix"
The problem is elsewhere. Asking the "Five Why's" might help locate it.
> lost count the number of times ... "I need to bypass this policy because shit's on fire and I've got a fix"
The problem is elsewhere. Asking the "Five Why's" might help locate it.
I've seen some pretty tortured answers to the five why's to avoid having to ask the really uncomfortable questions about a team/org/etc.
When "shit's on fire" usually multiple people are aware about it. In that case some of those should be able to jump in as reviewers and approve the changes without too much drama. In my experience that works fairly well - but it obviously depends on the team.
In such code reviews it can also help to point out
> this is a hotfix for issue XYZ. A better fix / more tests / etc will follow"
to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.
In such code reviews it can also help to point out
> this is a hotfix for issue XYZ. A better fix / more tests / etc will follow"
to make others aware why a change might not meet all best practices. And obviously if that is the case, the follow-up later on should really happen in order to avoid losing trust from reviewers.
Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.
> Where I currently work, we have "skip review" and "skip preflight" labels for this. The mergers have the power to merge anything anyway, the labels are only to make it an official request.
From the OP:
> Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
From the OP:
> Changes are categorized as either Ship (merge into mainline without review), Show (open a pull request for review, but merge into mainline immediately), or Ask (open a pull request for discussion before merging).
In the following paragraph:
> Do a bit more “Showing”, so you can release some of the pressure in your development pipeline. Then focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming. Every time a developer “Shows” rather than “Asks” is an opportunity for them to build trust with their team.
A team that builds trust is inviting the second pair of eyes. A team with no trust requires it.
> Do a bit more “Showing”, so you can release some of the pressure in your development pipeline. Then focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming. Every time a developer “Shows” rather than “Asks” is an opportunity for them to build trust with their team.
A team that builds trust is inviting the second pair of eyes. A team with no trust requires it.
It's not really a simple matter of trust, though. To take the example of the 'poka yoke' system that was posted here last week: is it because of a lack of trust that equipment operators are required to visually signal to themselves the action they will perform? Or is it because it's a simple and easy way to avoid basic operator error?
"We trust our engineers to merge code and not break production," is not the same as "we require peer review of changes to production, and/or QA, to mitigate human error."
Or more simply, trust but verify. Not every team will have a CI/CD pipeline that can effectively automate the 'verify' step.
Nobody on my team, for example, is worried about being required to have n approving reviews. If nothing else, it's a relief if it means we're not paged at 3am to fix something we deployed without fully checking.
"We trust our engineers to merge code and not break production," is not the same as "we require peer review of changes to production, and/or QA, to mitigate human error."
Or more simply, trust but verify. Not every team will have a CI/CD pipeline that can effectively automate the 'verify' step.
Nobody on my team, for example, is worried about being required to have n approving reviews. If nothing else, it's a relief if it means we're not paged at 3am to fix something we deployed without fully checking.
When it comes to coding, I don't trust my colleagues, but I don't trust myself either. If a second or third pair of eyes couldn't spot any issues in a PR, then it might be the correct solution.
Right, its like saying checklists are a sign of distrust. No, its an acknowledgement that good intentions dont fix human nature. Have an agreed upon process and stick to it.
Checklists are often a good thing; and an opportunity to optimize processes with team feedback!
"Post-surgical deaths in Scotland drop by a third, attributed to a checklist" https://news.ycombinator.com/item?id=19684376 https://westurner.github.io/hnlog/#comment-19684376
"Post-surgical deaths in Scotland drop by a third, attributed to a checklist" https://news.ycombinator.com/item?id=19684376 https://westurner.github.io/hnlog/#comment-19684376
But do you need that for every single commit? Sometimes things are so simple that code review isn't helpful. Such as:
1) Changing something in the README
2) Changing a typo
3) Changing the CSS color of something
Those are the very basic cases all orgs could benefit from doing in a no-review way. If you want to treat your developers like adults, and have some faith in the test suite they've written, you could also allow no-review for cases such as: 1) Developer fixes an obvious bug in an obvious way, writes unit tests to ensure that there are no regressions.
1) Changing something in the README
2) Changing a typo
3) Changing the CSS color of something
Those are the very basic cases all orgs could benefit from doing in a no-review way. If you want to treat your developers like adults, and have some faith in the test suite they've written, you could also allow no-review for cases such as: 1) Developer fixes an obvious bug in an obvious way, writes unit tests to ensure that there are no regressions.
If the change is extremely trivial then so is reviewing that change. Not much time is lost
Counterpoint: The actual review of an "extremely trivial" change may not take much time (seconds), but it can take orders of magnitude more time (minutes to hours) to coordinate and execute that review (notify team of pending review, wait, ping team, wait, ping individual devs, wait, ...).
I've spent the past decade working at a company that requires all changes to be reviewed no matter how trivial. Changes can be reviewed after the fact if the author marks it "to be reviewed". Doing that is rare, like <1% of changes.
I have never found this policy to significantly hamper my overall productivity both as an author or as a reviewer.
I have never found this policy to significantly hamper my overall productivity both as an author or as a reviewer.
So have I, except it's been three companies. One of them, ThoughtWorks, had built-in code review in the form of pair programming. I don't really have data on productivity, one way or the other, nor a strong opinion on the desirability or benefit of mandatory review. I was just addressing the specific comment of "Not much time is lost" above. Ç'est la guerre.
If it takes 5 seconds but happens after 10 minutes, then thee time from completing the development to review done is 10 minutes, not 5 seconds.
That's the problem with review of trivial typo fixes. Obviously no org should have those reviews as somehow so mandatory that each individual developer can't override the policy such as by self-reviewing the change.
If I was in an org where a policy required me to bug the dev next to me in order to approve a typo fix, I'd quite frankly quit.
That's the problem with review of trivial typo fixes. Obviously no org should have those reviews as somehow so mandatory that each individual developer can't override the policy such as by self-reviewing the change.
If I was in an org where a policy required me to bug the dev next to me in order to approve a typo fix, I'd quite frankly quit.
It's not about the time to review. It's about yet another interruption. For the sake of something trivial.
Agreed. I’m on a self-directed, 2-man dev team, and we chose to review every change. I often have obvious, stupid bugs in my code that a simple review catches. And at least as often, there’s some context or edge case that I’ve forgotten.
Regardless, my code is unquestionably better as a result of the process.
Regardless, my code is unquestionably better as a result of the process.
Spotting things the author missed is a low bar for code review. I believe the author assumes a context of high quality, well-tested code (e.g. "added a feature using an established pattern"). Unfortunately, for many teams code review is the only opportunity for improving code quality. If code quality is treated separately ("focus your efforts on activities that build trust, such as training, team discussions, or ensemble programming") then the importance of code review could be lessened.
I thought PRs also provided a way to train devs, and monitor their learning??
Also, having juniors PR senior's code gives the juniors a chance to see what (hopefully!) good code looks like?
Also, having juniors PR senior's code gives the juniors a chance to see what (hopefully!) good code looks like?
PR review time and effort is hard (not possible?) to track by project managers with tools like JIRA and doesn't contribute to a personal performance metrics, so naturally developers are trying to come up with a way to avoid doing it.
When you have a bad run-in with Goodhart's Law, don't blame the people who are simply doing their jobs as defined by the incentive structures they've been given. Blame the person who set up bad incentive structures.
> so naturally developers are trying to come up with a way to avoid doing it
I don't understand this. I very much want someone else to look at my work, because I know how prone I am to mistakes or overlooking something.
I don't understand this. I very much want someone else to look at my work, because I know how prone I am to mistakes or overlooking something.
The poster points to the fact that typically you have Jira tickets to write the code, and that’s assigned to you, and that adds to your velocity, but the reviewer of the code doesn’t get any credit for spending time on this. Thus in teams where the program manager doesn’t try to account for code review and they actually try to incentivize velocity (both stupid), you’re not incentivized to do thorough code reviews.
I think you've read that the wrong way round.
> I'm not following. We do reviews because a second pair of eyes can spot things the author missed.
Additionally, we (and many companies) have compliance standards that require a reviewer on everything.
Even if developers aren't acting maliciously, they can miss things. Particularly, if they're rushing (under tight deadlines) to ship something.
Additionally, we (and many companies) have compliance standards that require a reviewer on everything.
Even if developers aren't acting maliciously, they can miss things. Particularly, if they're rushing (under tight deadlines) to ship something.
Zero trust and multiple eyes on PRs is a good thing. No one’s code is 100% perfect all the time.
And of course this doesn’t apply to side projects or really small startups. Any mature company with a product where stability and security is important should have mandatory peer reviews in place.
And of course this doesn’t apply to side projects or really small startups. Any mature company with a product where stability and security is important should have mandatory peer reviews in place.
Reviews are also a good way to learn. Particularly seniors reviewing the code of juniors and providing feedback (code suggestions are great for this).
I think juniors reviewing the code of seniors - and feeling comfortable that nobody will judge them for asking questions - is even better.
Among other things, it introduces them to parts of the code they wouldn't otherwise get to touch, which helps them get a broader understanding of the context in which they're working. And it helps communicate that they, too, own the code and are full members of the team, not just peons in some hierarchy.
Among other things, it introduces them to parts of the code they wouldn't otherwise get to touch, which helps them get a broader understanding of the context in which they're working. And it helps communicate that they, too, own the code and are full members of the team, not just peons in some hierarchy.
Not just to spot issues but to make sure other developers on the team understand what else is being worked on. Spread the knowledge.
I think the article is written assuming that most reviews are just bureaucratic. In practice though not all codebases have 100% coverage integ tests, perf regression tests and visual regression tests. Even then I feel you should still have a second pair of eyes but I agree with the underlying problem that code review is a development bottleneck.
My take is I expect "reviews" of every code change. whereas an "ask" is more based in the context of what I am currently doing.
Crappy example:
1 - Adding a user to a group, I expect a review - did I get the name correct?
2 - Adding a group, I expect a review & an ask - does the group make sense for our context, and is it named correctly.
Crappy example:
1 - Adding a user to a group, I expect a review - did I get the name correct?
2 - Adding a group, I expect a review & an ask - does the group make sense for our context, and is it named correctly.
Change Control has historically been "Change Prevention" in many organizations. They optimized for stability
It also helps spread knowledge around so more than one person has knowledge of a new feature, bug fix, or sub-system.
Honestly -- I work in a regulated environment. We'd fail audits without 2 :eyes: on every PR
I can't see how you can get any non-tribal code in the codebase without asking.
In my language the military has a saying « trust to not exclude control »
Simple and to the point.
Simple and to the point.
That's 100% correct, it shows a lack of trust in myself, because I know that I am a failable person who occasionaly writes code under conditions of stress and lack of sleep and appreciates having my coworkers sanity check my work.
Honestly if anything, code reviews show an abundance of trust in the development team because I trust that my coworkers will see things that I did not.
Honestly if anything, code reviews show an abundance of trust in the development team because I trust that my coworkers will see things that I did not.
Bad idea. I think this bit near the top shows that the premise is incorrect:
the adoption of Pull Requests as the primary way of contributing code has created problems. We’ve lost some of the “Ready to Ship” mentality we had when we did Continuous Integration. Features-in-progress stay out of the way by delaying integration, and so we fall into the pitfalls of low-frequency integration that Continuous Integration sought to address.
PRs and code reviews don't need to cause major slowdowns. You should be able to commit multiple PRs on the same day when you need to, and if a single PR takes more than a couple of days, that's probably a sign that you should split the work into smaller chunks (probably using techniques from elsewhere on martinfowler.com!)
There definitely are times when you think "argh, I just want to fix that one comment / typo!" without bothering somebody for a code review. But where is the threshold where that's acceptable? Changing one line of code? One function?
It seems very clear to me, and I'd be very interested to hear arguments against this, that there is no threshold and you should always get a code review (assuming an established codebase with multiple developers). Edit to add: I could see maybe making an exception for specific non-code files, like READMEs.
The reason I don't want a review is not because I don't want to bother somebody -- nobody really minds being asked -- it's because I'm embarrassed at the silly mistake that slipped through! In that situation, pushing the one-line fix without review is a bad habit, and I need to be honest to my colleagues and ask somebody to wave through the quick fix. And if that happens a lot, I should slow down just a little bit and try to avoid making those little mistakes so often.
the adoption of Pull Requests as the primary way of contributing code has created problems. We’ve lost some of the “Ready to Ship” mentality we had when we did Continuous Integration. Features-in-progress stay out of the way by delaying integration, and so we fall into the pitfalls of low-frequency integration that Continuous Integration sought to address.
PRs and code reviews don't need to cause major slowdowns. You should be able to commit multiple PRs on the same day when you need to, and if a single PR takes more than a couple of days, that's probably a sign that you should split the work into smaller chunks (probably using techniques from elsewhere on martinfowler.com!)
There definitely are times when you think "argh, I just want to fix that one comment / typo!" without bothering somebody for a code review. But where is the threshold where that's acceptable? Changing one line of code? One function?
It seems very clear to me, and I'd be very interested to hear arguments against this, that there is no threshold and you should always get a code review (assuming an established codebase with multiple developers). Edit to add: I could see maybe making an exception for specific non-code files, like READMEs.
The reason I don't want a review is not because I don't want to bother somebody -- nobody really minds being asked -- it's because I'm embarrassed at the silly mistake that slipped through! In that situation, pushing the one-line fix without review is a bad habit, and I need to be honest to my colleagues and ask somebody to wave through the quick fix. And if that happens a lot, I should slow down just a little bit and try to avoid making those little mistakes so often.
> But where is the threshold where that's acceptable? Changing one line of code? One function?
I think this is the key part where one simply should trust developers to do the right thing. I "self-approve" typo fixes. I might even self approve small enough code fixes. But the key is - I choose that myself and there is a treshold. It might be slightly different for each developer, and that's of course both good and bad.
But regardless - the reason it has to be that way, is because the alternative is unacceptable: where the process is so rigid that every trivial change such as a typo MUST be approved by a second person. Having people draw arbitrary lines for what kind of complexity warrants a review might sound bad, but it's infinitely better than the alternative.
I think this is the key part where one simply should trust developers to do the right thing. I "self-approve" typo fixes. I might even self approve small enough code fixes. But the key is - I choose that myself and there is a treshold. It might be slightly different for each developer, and that's of course both good and bad.
But regardless - the reason it has to be that way, is because the alternative is unacceptable: where the process is so rigid that every trivial change such as a typo MUST be approved by a second person. Having people draw arbitrary lines for what kind of complexity warrants a review might sound bad, but it's infinitely better than the alternative.
I sometimes self-approve small changes too. But I really do think it's a bad habit and unnecessary. I don't like hassling people with tiny trivial code reviews, but I don't mind at all when people ask me for the same, so I don't think it is actually a hassle.
Catching bugs is only one of the reasons for doing code reviews (and not even a great reason at that -- plenty of bugs make it through code review after all). Another important reason is sharing knowledge within the team, and that applies to tiny changes as much as large ones.
I don't think mandatory code reviews regardless of size are really "rigid" or "unacceptable" at all, not in a good team where code reviews are a well-established part of the culture. In jobs where I didn't have admin privileges and wasn't able to bypass code review even for trivial changes, we reviewed absolutely everything and got on just fine.
Sure, sometimes the reviewer barely looks at the change and just rubber-stamps it. But that's a sign of trust, which is a good thing, and still gives you that little bit of team cohesion and knowledge sharing -- at least one other person is roughly aware of what you're doing.
Catching bugs is only one of the reasons for doing code reviews (and not even a great reason at that -- plenty of bugs make it through code review after all). Another important reason is sharing knowledge within the team, and that applies to tiny changes as much as large ones.
I don't think mandatory code reviews regardless of size are really "rigid" or "unacceptable" at all, not in a good team where code reviews are a well-established part of the culture. In jobs where I didn't have admin privileges and wasn't able to bypass code review even for trivial changes, we reviewed absolutely everything and got on just fine.
Sure, sometimes the reviewer barely looks at the change and just rubber-stamps it. But that's a sign of trust, which is a good thing, and still gives you that little bit of team cohesion and knowledge sharing -- at least one other person is roughly aware of what you're doing.
I am surprised by the reactions. The author is just proposing a workflow where PRs are optional. For many years we worked without PRs at all.
It bothers me that it seems like developers no longer do "Continuous Integration" which IMHO is one of the most important software engineering practices. Companies do CI/CD pipelines, but that is not even close to "Continuous Integration"
It bothers me that it seems like developers no longer do "Continuous Integration" which IMHO is one of the most important software engineering practices. Companies do CI/CD pipelines, but that is not even close to "Continuous Integration"
- Maintain a Single Source Repository.
- Automate the Build
- Make Your Build Self-Testing
- Everyone Commits To the Mainline Every Day
- Every Commit Should Build the Mainline on an Integration Machine
- Fix Broken Builds Immediately
- Keep the Build Fast
- Test in a Clone of the Production Environment
- Make it Easy for Anyone to Get the Latest Executable
- Everyone can see what's happening
- Automate Deployment
Love your comment. People seem to have forgotten what "Continuous Integration" stands for.
I am amazed to see so much "mandatory CR is good" comments on HN.
Well yes, it's fine for open source projects or immature teams working on mature projects without good delivery pipeline.
Well yes, it's fine for open source projects or immature teams working on mature projects without good delivery pipeline.
"Everyone commits to the mainline every day" doesn't preclude Pull Requests. It just means "don't use large multi-day pull requests" really. And that's a good idea even though I wouldn't want ALL PR's to be sub 1 day of effort.
> For many years we worked without PRs at all.
And that severely reduced the amount of learning-from-others that went on in your company during that era.
Discussion in the course of code reviews is the most effective way for less experienced programmers to benefit from their more experienced colleagues. And, of course, even the most experienced programmers continue to learn from their colleagues thoughts.
And that severely reduced the amount of learning-from-others that went on in your company during that era.
Discussion in the course of code reviews is the most effective way for less experienced programmers to benefit from their more experienced colleagues. And, of course, even the most experienced programmers continue to learn from their colleagues thoughts.
> And that severely reduced the amount of learning-from-others that went on in your company during that era.
I was not talking just about my self. I was talking about the whole industry. PRs became popular when github became popular; about 10 years ago.
I was not talking just about my self. I was talking about the whole industry. PRs became popular when github became popular; about 10 years ago.
This is wrong. Code review is not for "asking". It has many benefits that the author is missing:
1. Get extra eyes on a change.
2. Learn from your colleagues' thoughts on the work.
Honestly, the most important ones may be 2. Almost everything I've learned as a software engineer comes from thoughtful code review comments left by the skilled colleagues with whom I was lucky to work (back when my company was more desirable!)
I wish the same benefit on every other aspiring software engineer out there. This article threatens to disinherit them and deny them that source of learning.
But it's not just less experienced engineers who benefit from thoughtful comments on all substantial PRs; we all do. I never want to work somewhere that discourages code review like this.
To prevent code review causing slowness there's a simple rule: everybody does their code review as top priority.
It is true that PRs can make inexperienced engineers think it's ok to open a PR a branch that they haven't carefully tested and that isn't ready to ship. Unless they mark it as WIP, that's not acceptable, as the article correctly says. So, you have to educate people there.
1. Get extra eyes on a change.
2. Learn from your colleagues' thoughts on the work.
Honestly, the most important ones may be 2. Almost everything I've learned as a software engineer comes from thoughtful code review comments left by the skilled colleagues with whom I was lucky to work (back when my company was more desirable!)
I wish the same benefit on every other aspiring software engineer out there. This article threatens to disinherit them and deny them that source of learning.
But it's not just less experienced engineers who benefit from thoughtful comments on all substantial PRs; we all do. I never want to work somewhere that discourages code review like this.
To prevent code review causing slowness there's a simple rule: everybody does their code review as top priority.
It is true that PRs can make inexperienced engineers think it's ok to open a PR a branch that they haven't carefully tested and that isn't ready to ship. Unless they mark it as WIP, that's not acceptable, as the article correctly says. So, you have to educate people there.
Weakest part of CI/CD workflows is when code requires corresponding infrastructure change, such as creation or modification of S3 bucket, IAM permission or pubsub creation. Worst part is when such changes require carefull orchestration.
I am yet to see a development workflow, capable of developing and releasing something as simple as no dowmtime migration of API to another load balancer.
I am yet to see a development workflow, capable of developing and releasing something as simple as no dowmtime migration of API to another load balancer.
Well that's because it's not a development workflow. That's a live systems change. It's like changing the tire on a moving vehicle.
If you have a very repeatable environment, you can have an entire pipeline that creates new infra from scratch (w/Terraform), build and deploy your new app, test it, and then point traffic at the new infra. It's like blue/green but bigger. You aren't changing the tire, you're moving from one moving vehicle to another one. That works well because there's no chance for unusual problems from trying to figure out how to re-jigger things on the fly.
The former is configuration-management-organized infrastructure, and the latter is immutable infrastructure.
The problem comes in with things like changing an S3 bucket or IAM role. Changing those things is like changing the highway... you can't replace the highway. You have to close down a lane of traffic, put up traffic cones, reduce the speed limit, make your changes carefully. Ideally test on a strip of test highway first.
These cloud-managed services cannot be made immutable, so you have to use configuration-management. So you have to have a change management system in place, and tightly manage the dependency between your app and the change.
If you have a very repeatable environment, you can have an entire pipeline that creates new infra from scratch (w/Terraform), build and deploy your new app, test it, and then point traffic at the new infra. It's like blue/green but bigger. You aren't changing the tire, you're moving from one moving vehicle to another one. That works well because there's no chance for unusual problems from trying to figure out how to re-jigger things on the fly.
The former is configuration-management-organized infrastructure, and the latter is immutable infrastructure.
The problem comes in with things like changing an S3 bucket or IAM role. Changing those things is like changing the highway... you can't replace the highway. You have to close down a lane of traffic, put up traffic cones, reduce the speed limit, make your changes carefully. Ideally test on a strip of test highway first.
These cloud-managed services cannot be made immutable, so you have to use configuration-management. So you have to have a change management system in place, and tightly manage the dependency between your app and the change.
You've never seen a blue/green deployment or a DNS change without downtime?
I've never seen _workflow_, were developer single handedly can create a well tested PR performing all necessary release steps upon merge.
At least for cloud environments, I haven't seen a company that doesn't have such a workflow in place for a long time.
What exactly goes into PRs you've seen? How do they codify these steps, such that:
- Release steps can be tested while PR is being developed. That is executed somewhere, which doesn't interfere with production or ideally other developers ongoing PR should it go bad
- Upon merge production system safely transitioned from one state to another without human intervention.
Steps, for simplicity are:
- Deploy new version of the code behind new load balancer
- Make some requests through it to verify its workig
- Switch DNS to the new load balancer IP
- Wait for old load balancer new connections to die down and remove both LB and old version of the code behind it.
- Release steps can be tested while PR is being developed. That is executed somewhere, which doesn't interfere with production or ideally other developers ongoing PR should it go bad
- Upon merge production system safely transitioned from one state to another without human intervention.
Steps, for simplicity are:
- Deploy new version of the code behind new load balancer
- Make some requests through it to verify its workig
- Switch DNS to the new load balancer IP
- Wait for old load balancer new connections to die down and remove both LB and old version of the code behind it.
I agree, I've started to demerge those sorts of changes. Infra code moves slower (over the long term) than app code. So having seperate pipelines is IMO better.
Though on your final comment, I'd say doing something like a no down time api migration is actually pretty complicated if you're not doing it a lot.
Though on your final comment, I'd say doing something like a no down time api migration is actually pretty complicated if you're not doing it a lot.
Perhaps a better strategy for code reviews is "LGTM with nits".
1. Every change should go through code review
2. Changes should be small, to make them easier to review, less risky, and to promote continuous integration
3. Team should prioritize review latency
4. Reviewers should LGTM/approve if there are no major issues with the change, trusting the author to resolve any minor/nit comments
https://dev.chromium.org/developers/contributing-code/minimi...
1. Every change should go through code review
2. Changes should be small, to make them easier to review, less risky, and to promote continuous integration
3. Team should prioritize review latency
4. Reviewers should LGTM/approve if there are no major issues with the change, trusting the author to resolve any minor/nit comments
https://dev.chromium.org/developers/contributing-code/minimi...
I like that! I've done approved with suggestions for a long time, it strikes a good balance I think - and I've long been an advocate of fast reviews as a priority.
> The reason you’re reliant on a lot of “Asking” might be that you have trust issue. “All changes must be approved” or “Every pull request needs 2 reviewers” are common policies, but they show a lack of trust in the development team.
In my current company, teams are left to come up with their own policy, but most teams I know of have the "every commit needs 2 reviewers" policy (the ones that don't have a "1 reviewer policy"). I am a Principal Engineer at a big-tech company, and when I want to commit code, I must get it reviewed by 2 engineers, same as an engineer who graduated in May. It's not because my team doesn't trust me (I think...), but rather I earn my team's trust by showing that my seniority doesn't put me above the rules. My seniority also doesn't put me above getting better either.
Prior to my current company, most places I've worked generally had all code reviews optional. It was usually the better engineers that were more likely to ask for a review.
> The reason you’re reliant on a lot of “Asking” might be that you have trust issue.
I'd flip this on the head - the reason many teams are reliant on skipping code reviews is that they have a problem doing code reviews. As engineers, reviewing each other's work is part of our job. We should be prioritizing it as such. On the team's that I've been on that prioritize doing code reviews, waiting for a review has hardly ever been a problem.
In my current company, teams are left to come up with their own policy, but most teams I know of have the "every commit needs 2 reviewers" policy (the ones that don't have a "1 reviewer policy"). I am a Principal Engineer at a big-tech company, and when I want to commit code, I must get it reviewed by 2 engineers, same as an engineer who graduated in May. It's not because my team doesn't trust me (I think...), but rather I earn my team's trust by showing that my seniority doesn't put me above the rules. My seniority also doesn't put me above getting better either.
Prior to my current company, most places I've worked generally had all code reviews optional. It was usually the better engineers that were more likely to ask for a review.
> The reason you’re reliant on a lot of “Asking” might be that you have trust issue.
I'd flip this on the head - the reason many teams are reliant on skipping code reviews is that they have a problem doing code reviews. As engineers, reviewing each other's work is part of our job. We should be prioritizing it as such. On the team's that I've been on that prioritize doing code reviews, waiting for a review has hardly ever been a problem.
How would this kind of system handle: “your change works but the way you implemented it breaks X Y and future Z, now there are 10 commits on top of it and we have to roll back.” Seems like it would create a code base with a lot of hacks or need constant refactoring.
If something breaks with the change, how can you say "your change works"? Either it works and everything works, or it doesn't work, and something/everything else broke. And since it never worked in the first place, it wouldn't be merged to the "ship" branch and therefore never get any commits on top.
In theory yes. In practice, your test suites are not all-encompassing perfect oracles.
Hopefully you are a proper software shop and you do more than just run test suites to see if something is working :)
And if you discover that "hey, changing X broke Y but we didn't catch it before", then obviously you rollback the change and start working towards how you can prevent it in the future.
Of course there will always be fuckups, but this strategy wouldn't make it harder/easier to rollback those changes.
And if you discover that "hey, changing X broke Y but we didn't catch it before", then obviously you rollback the change and start working towards how you can prevent it in the future.
Of course there will always be fuckups, but this strategy wouldn't make it harder/easier to rollback those changes.
Right, but it removes the opportunity for a second dev to see subtle but catchable issues that are the most likely to get missed in isolated testing by dev/first level QA. Race conditions, improper use of lifecycle hooks, scaling issues, etc.
And good luck getting a SOX auditor to give this process an OK!
And good luck getting a SOX auditor to give this process an OK!
Interesting; moves a lot closer to zero-friction yolo-merging with zero review. I get the impression that many of the FAANG companies have deployment processes (e.g. blue/green) which allow that without too much risk.
I would note that anyone outwith the web world will have a lot more trouble if it's at all difficult to "undeploy" software in the event of a problem.
I would note that anyone outwith the web world will have a lot more trouble if it's at all difficult to "undeploy" software in the event of a problem.
Even inside in the web world, this approach wouldn't quite work for features with even a modicum of state.
I agree. I think the author is projecting an opinion of "I don't want to do code reviews" so is justifying that opinion by outlining a process which allows him to bypass reviews in majority of cases.
My own opinion: do code reviews. They are a good thing. However if you don't want to or cannot make all the suggested fixed before merging then put the comments into s new ticket for later and move on. Just don't skip the process and lose that chance to have a record of any technical debt you've incurred.
My own opinion: do code reviews. They are a good thing. However if you don't want to or cannot make all the suggested fixed before merging then put the comments into s new ticket for later and move on. Just don't skip the process and lose that chance to have a record of any technical debt you've incurred.
> I think the author is projecting an opinion of "I don't want to do code reviews" so is justifying that opinion by outlining a process which allows him to bypass reviews in majority of cases.
I thought that too. The author seems to came up with the whole "Ship/ Show/ Ask" strategy so that it looks like their team actually follows a process instead of just plain skipping on doing code reviews.
While I can relate to their motivation to go without code reviews:
> Sometimes Pull Requests sit around and get stale, or we’re not sure what to work on while we wait for review...
> We also get tired of the number of Pull Requests we have to review, so we don't talk about the code anymore. We stop paying attention and we just click “Approve” or say “Looks good to me”.
These problems are very real and take time and effort to address. I just don't think opting out of reviews is a good way to solve them.
Well, the team I'm on accepts what the blog post calls a Show PR in a rare occasion when something has to be fixed right now. E.g., the prod is down kind of situation. We still go through a review but don't wait for approvals to merge and if there is any feedback we deal with it later.
I thought that too. The author seems to came up with the whole "Ship/ Show/ Ask" strategy so that it looks like their team actually follows a process instead of just plain skipping on doing code reviews.
While I can relate to their motivation to go without code reviews:
> Sometimes Pull Requests sit around and get stale, or we’re not sure what to work on while we wait for review...
> We also get tired of the number of Pull Requests we have to review, so we don't talk about the code anymore. We stop paying attention and we just click “Approve” or say “Looks good to me”.
These problems are very real and take time and effort to address. I just don't think opting out of reviews is a good way to solve them.
Well, the team I'm on accepts what the blog post calls a Show PR in a rare occasion when something has to be fixed right now. E.g., the prod is down kind of situation. We still go through a review but don't wait for approvals to merge and if there is any feedback we deal with it later.
> My own opinion: do code reviews. They are a good thing.
Being a solo dev at the moment, I sorely wish I had someone to review my changes. Have only had one major mistake slip through in ~2 months (and it wasn't a bad one) but still, someone else could have caught that.
Being a solo dev at the moment, I sorely wish I had someone to review my changes. Have only had one major mistake slip through in ~2 months (and it wasn't a bad one) but still, someone else could have caught that.
I'm sure you could hire someone to help (without googling I'm willing to be pr-as-a-service exists) , but you'd end up being unable to call yourself a solo dev!
Oh, sorry, I should clarify - I'm working for a company that are having trouble hiring and retaining backend devs which is why I'm the only one here atm.
Same here. In addition I also can see reviewing other people's code as education.
It is normal to accept that it broke something and simply push a fix instead of rolling back. Once something is "out" a rollback can significantly complicate things both internally and externally for the end users.
kinda disappointing to see this kind of content posted on Fowler's site.
pragmatically, the only case I could see for doing this kind of flow is for a small project internal to a team that has no external entities that rely on it, so when things break it isn't borking shit for anyone else.
even then, its a borderline red flag.
no way do I trust myself to catch all edge cases, that's what code review is for.
pragmatically, the only case I could see for doing this kind of flow is for a small project internal to a team that has no external entities that rely on it, so when things break it isn't borking shit for anyone else.
even then, its a borderline red flag.
no way do I trust myself to catch all edge cases, that's what code review is for.
There isn't really anything in the code that suggests that the "mainline" mentioned is automatically deployed to production or deployed to anything at all. The article mentions CI/CD but that doesn't necessarily mean lights will go out if there are mistakes.
It all depends on what happens once code reaches mainline. For a desktop app, you might be a month from any production use of your code, and still use "CI/CD" where the CD is simply a nightly build, for example.
It all depends on what happens once code reaches mainline. For a desktop app, you might be a month from any production use of your code, and still use "CI/CD" where the CD is simply a nightly build, for example.
If you don't trust yourself, the domain expert on a piece of code, how do you trust someone else, who has less context, and their own work to do?
I think perhaps its a fair point for junior devs, who either don't have a good idea of how their changes might impact things broadly, or don't have the experience/developed aesthetic to know what is good/idiomatic/performant/code-smelly.
I think perhaps its a fair point for junior devs, who either don't have a good idea of how their changes might impact things broadly, or don't have the experience/developed aesthetic to know what is good/idiomatic/performant/code-smelly.
TL;DR - Two sets of eyes is better than one.
One of the most important aspects of code review culture isn't really mentioned in the post: requiring at least one additional approval on a PR ensures that at least 2 people are familiar with a piece of code. This helps increase the bus factor on the project and ensures that knowledge transfer and developer velocity can be maintained.
Additionally, the dialogue that occurs during code review can serve as additional documentation into why certain choices were made. I use the blame layer all of the time to try to better understand the context of a particular piece of code. Being able to read through the review for the PR that contained a line of code is often incredibly useful for being able to gain context.
One of the most important aspects of code review culture isn't really mentioned in the post: requiring at least one additional approval on a PR ensures that at least 2 people are familiar with a piece of code. This helps increase the bus factor on the project and ensures that knowledge transfer and developer velocity can be maintained.
Additionally, the dialogue that occurs during code review can serve as additional documentation into why certain choices were made. I use the blame layer all of the time to try to better understand the context of a particular piece of code. Being able to read through the review for the PR that contained a line of code is often incredibly useful for being able to gain context.
Have you never had a problem caught by a PR reviewer?
Great idea, everyone merges to master without a second pair of eyes catching that subtle bug in a fundamental function that causes data corruption over the course of a month before a customer notifies you about it because there is no test for that particular case, except now mainline is over 500 commits and 10 features ahead and 6 of those depend on that function plus there have been 5 schema changes in the meantime, two of which can't be rolled back without losing other customer data.
Great Idea.
Great Idea.
Thanks. I hate it.
As the saying goes, we all have a testing tier. Some of us also have a separate tier for production.
As the saying goes, we all have a testing tier. Some of us also have a separate tier for production.
The problems I see with the Ship / Show / Ask approach are
- How often each is selected will be *very* personality-based and I'm afraid the worst ones will be magnified. AKA "I'm so super good, all my changes are Ship" kinda persons.
- It becomes harder to justify imposing a mode on someone. How do you rein in the ultra-shippers?
- It can become a source of frustration and strife between co-worker. Not just due to the above, but when someone breaks the builds with a Ship and people will start keeping count how many time X broke it, etc.
- With time, I feel there will be drift into less and less Ask.
I reviewed the article in question and have a little more color to add for some of the skeptics commenting below.
Advocates of pre-merge review point out (correctly) that peer review is valuable: humans are fallible, and a second pair of eyes often helps. Maybe I read a different article, but I don't think the author disputes this. What gets lost in the discourse around the dominant PR-based, asynchronous workflow is that it comes with tradeoffs. Do you understand what you're giving up to get back in your preferred mode of working? Are you so certain it's more appropriate for your present circumstances?
_Forced_ pre-merge review has a number of negative tradeoffs that aren't always visible to the teams that use it. For one thing, it can lead to "review theatre": casual pull request reviews can't meaningfully detect most bugs; reviews that can are hugely time consuming and as a result quite rare; poor PRs are sometimes "laundered" by the review process; poor reviewers encourage bikeshedding; but even a bad review can introduce a cycle time hit and a bunch of context-switching as both the submitter and reviewer bounce back and forth. If you work at a shop that uses PRs and has none of these problems, I salute you; I have not.
The answer to all of these from PR advocates tends to be, well, maybe make the pre-merge review process itself better, to which I say: you are making a slow process slower; if your team is trying to move quickly, instead of adding additional padding around a slow process, maybe try to smooth out a fast one?
A good framing question I ask my teams is: if your goal was to get high quality code into production as often as possible, what processes would you tweak and why? Where would you invest and where would you pull back? There are lots of great ways to ship high-quality code quickly without pull requests; we did it all the time before they were invented.
Advocates of pre-merge review point out (correctly) that peer review is valuable: humans are fallible, and a second pair of eyes often helps. Maybe I read a different article, but I don't think the author disputes this. What gets lost in the discourse around the dominant PR-based, asynchronous workflow is that it comes with tradeoffs. Do you understand what you're giving up to get back in your preferred mode of working? Are you so certain it's more appropriate for your present circumstances?
_Forced_ pre-merge review has a number of negative tradeoffs that aren't always visible to the teams that use it. For one thing, it can lead to "review theatre": casual pull request reviews can't meaningfully detect most bugs; reviews that can are hugely time consuming and as a result quite rare; poor PRs are sometimes "laundered" by the review process; poor reviewers encourage bikeshedding; but even a bad review can introduce a cycle time hit and a bunch of context-switching as both the submitter and reviewer bounce back and forth. If you work at a shop that uses PRs and has none of these problems, I salute you; I have not.
The answer to all of these from PR advocates tends to be, well, maybe make the pre-merge review process itself better, to which I say: you are making a slow process slower; if your team is trying to move quickly, instead of adding additional padding around a slow process, maybe try to smooth out a fast one?
A good framing question I ask my teams is: if your goal was to get high quality code into production as often as possible, what processes would you tweak and why? Where would you invest and where would you pull back? There are lots of great ways to ship high-quality code quickly without pull requests; we did it all the time before they were invented.
The article also encourages other methods of involving your colleages. "Talk to your team before you start, so you can get better ideas and avoid rework."
So if you do that or even pair program it will be easy for the involved (if you involved them good enough) to review.
People seem to vastly underestimate the (accidental) complexity introduced by branching and delaying integration, and overestimate the risk of integrating and releasing the changes, which is literally the essence of professional software development.
Your job from the business perspective is getting the changes into production. Facilitating that makes high-performing teams. Think about what you would need in order to safely merge to main multiple times a day. Trustworthy test suite? "Extra pair of eyes but without the wait and comment ping-pong", also known as "pair programming"? Build automation? Great deployment scripts? Trusted feature toggles? Shared code ownership?
Doing "feature branching" instead of that you get exotic branching strategies, sophisticated infrastructure to deploy your system from any desired branch, PRs hanging for weeks, even months, testing features in isolation, required approval rules, big merge conflicts... and a dumpster fire when something inevitably breaks and it turns out eventually you HAVE TO merge that hotfix to main branch QUICKLY.
I've worked in teams that mandate double code review, for reasons of "safety and code quality". Guess what. Code quality was shit anyway, and deployments were done infrequently, always with the feeling that it's risky.
I've helped same teams to simplify the workflow and deliver more value by just building the trust and gradually transitioning from "ask" to "show" & "ship" approach. I like the article a lot, since it describes something I've seen to emerge in reality.
But don't take my word on it, watch this instead: https://www.youtube.com/watch?v=v4Ijkq6Myfc
Your job from the business perspective is getting the changes into production. Facilitating that makes high-performing teams. Think about what you would need in order to safely merge to main multiple times a day. Trustworthy test suite? "Extra pair of eyes but without the wait and comment ping-pong", also known as "pair programming"? Build automation? Great deployment scripts? Trusted feature toggles? Shared code ownership?
Doing "feature branching" instead of that you get exotic branching strategies, sophisticated infrastructure to deploy your system from any desired branch, PRs hanging for weeks, even months, testing features in isolation, required approval rules, big merge conflicts... and a dumpster fire when something inevitably breaks and it turns out eventually you HAVE TO merge that hotfix to main branch QUICKLY.
I've worked in teams that mandate double code review, for reasons of "safety and code quality". Guess what. Code quality was shit anyway, and deployments were done infrequently, always with the feeling that it's risky.
I've helped same teams to simplify the workflow and deliver more value by just building the trust and gradually transitioning from "ask" to "show" & "ship" approach. I like the article a lot, since it describes something I've seen to emerge in reality.
But don't take my word on it, watch this instead: https://www.youtube.com/watch?v=v4Ijkq6Myfc
This could work in what I'll call a Fowlerian model for software development. This means that the code has superb unit testing and coverage; and that functionality is protected behind feature toggles rather than as artifact deployment.
That model has its advantages - but requires a mature and disciplined team behind it to not mess everything up and write crap and talk about it later (if at all). Functionality is exposed when the feature toggle is enabled - not when the CD system deploys it.
I would have trouble with exposing new functionality without a review of any sort - even on my own (perfect) code.
This can work in that Fowlerian ideal and possibly in the consultancy that he works for where it's got a mature and disciplined team. The issue is that I've found it to be rare where that sort of maturity and discipline exists - and that the developers aren't under a schedule pressure that has them submit less than ideal code to be fixed up later.
"Hey! It complies! Ship it!" is meant to be humorous.
That model has its advantages - but requires a mature and disciplined team behind it to not mess everything up and write crap and talk about it later (if at all). Functionality is exposed when the feature toggle is enabled - not when the CD system deploys it.
I would have trouble with exposing new functionality without a review of any sort - even on my own (perfect) code.
This can work in that Fowlerian ideal and possibly in the consultancy that he works for where it's got a mature and disciplined team. The issue is that I've found it to be rare where that sort of maturity and discipline exists - and that the developers aren't under a schedule pressure that has them submit less than ideal code to be fixed up later.
"Hey! It complies! Ship it!" is meant to be humorous.
Agreed. An excelleent team working on excellent code can use any process they want and be successful.
That's why the interesting process discussion is really: "what works for a team of sloppy and mediocre developers?".
That's why the interesting process discussion is really: "what works for a team of sloppy and mediocre developers?".
Every time I see some type of branching strategy it's always about main/master. Don't people have multiple environments, such as dev, test, pre-prod, prod? These strategies always apply merge/pull requests or changes directly to master. I protect every remote branch and require merge/pull requests to start on dev (unless of a hotfix.)
Maintaining a branch for every environment is extremely complex and error prone. All the branching strategies you see are always about main because it's simpler. Ensure the code merged into the main branch is always in a working state and you don't need to have a complex branching strategy.
This requires stricter testing practices in the PRs, and usually also involves either requiring PRs to be up to date with master prior to merge, or re-testing of master after merge, prior to the SHA being deployable.
This requires stricter testing practices in the PRs, and usually also involves either requiring PRs to be up to date with master prior to merge, or re-testing of master after merge, prior to the SHA being deployable.
I generally setup CI to build from master and push to the test environment, then I manually promote the same artifact to prod when ready. This way there's no big "merge dev to prod" PRs, and the binary that runs in prod is 100% the same one tested in pre-prod.
git flow, arguably the only branching strategy people talk about by name, is all about maintaining multiple environments [1]
test environment is where PRs are deployed (or just tested) before being merged
pre-prod is for the release branch and prod is for the master branch
[1] https://nvie.com/posts/a-successful-git-branching-model/
test environment is where PRs are deployed (or just tested) before being merged
pre-prod is for the release branch and prod is for the master branch
[1] https://nvie.com/posts/a-successful-git-branching-model/
Git Flow is not the only game in town! Take a look at GitLab Flow (and its variants), this intro does a good job of comparing related options (including git flow) --
https://docs.gitlab.com/ee/topics/gitlab_flow.html
https://docs.gitlab.com/ee/topics/gitlab_flow.html
My thinking is that you have one or more dev environments and the engineers are free to deploy whatever branch they want to these places whenever. If you have manual qa or some other team doing pre release qa, then merging your change to main can deploy to a stage env, and once the qa team okays it, the release propagates to prod. The stage env is thus optional.
But are you using a different branch for stage evn. As in, I wouldn't want to merge a release into master that gets deployed to a stage env. Because, if a critical hotfix is needed on master (for prod env) then I can't deploy prod again because master now has a new release in it (because of the stage env.)
Master doesn't have to be the only branch that deploys to prod, you could create a hotfix branch (starting from an earlier commit) and set it to push to prod in your CI to get an emergency release out.
Environments are generally handled via deployment configuration.
Whither pair programming?
There was a period early in the agile movement where code review was eschewed as overly bureaucratic. The solution to get similar gains (nay, better!) was to do pair programming.
My personal experience with it was that when it worked it indeed worked well. Way better than “can you look at this/approve it for me?” But I also admit I haven’t done any real amount of pair programming in years now.
Does anyone effectively transcend reviews with pair programming anymore? Or is it a hippy coder thing of the past?
There was a period early in the agile movement where code review was eschewed as overly bureaucratic. The solution to get similar gains (nay, better!) was to do pair programming.
My personal experience with it was that when it worked it indeed worked well. Way better than “can you look at this/approve it for me?” But I also admit I haven’t done any real amount of pair programming in years now.
Does anyone effectively transcend reviews with pair programming anymore? Or is it a hippy coder thing of the past?
I've long been intrigued by pair programming but I've never worked at a company where it was widely used.
The few times I've been able to do it, people were generally reluctant to commit code without another code review, which struck me as missing the whole point of pairing.
The few times I've been able to do it, people were generally reluctant to commit code without another code review, which struck me as missing the whole point of pairing.
Coming into a "enterprise" organisation this is a decent generalisation of the pains of opening a PR.
Any reccomendations of how to implement this? Tags would work but only if you understand the context of ship/show/ask.
100% opening a PR, regardless if its literally just merge this badboy ends up being a "show/ask" instead of a "ship" where i am currently working
Any reccomendations of how to implement this? Tags would work but only if you understand the context of ship/show/ask.
100% opening a PR, regardless if its literally just merge this badboy ends up being a "show/ask" instead of a "ship" where i am currently working
What I take from this approach is that you might want to leave an option for automatic merge/change application after CI green lighting. So a documentation change could be tagged as such and go live directly.
But do you really need a framework for that? Should not every developer be able to bypass the code review any time when necessary?
But do you really need a framework for that? Should not every developer be able to bypass the code review any time when necessary?
Allowing developers to merge directly into any mainline is a bad idea. Everyone makes mistakes here and there. I have senior developers STILL committing things they didn't intend to (that have been using git for years), and so a chance for someone to review it helps. But maybe they still make mistakes because they know someone else is going to look at it, I don't know.
I disabled push on master for everyone (includes myself). There are just damn so many `forgot to create a branch and pushed directly on master` type accidents.
A auto approved PR at least allows you to know who made the change at when. Push directly don't really preserve the information.
A auto approved PR at least allows you to know who made the change at when. Push directly don't really preserve the information.
I'll coin another low-review approach that I'll call:
Ship/Tow/Sink
Just make the change, recruit some help to put out the fire, watch the project die. /s
In all honesty some good points here, we definitely do a variation of this - but I think the emphasis should probably be on show/ask - and mostly ask (two pairs of eyes are better than one).
Ship/Tow/Sink
Just make the change, recruit some help to put out the fire, watch the project die. /s
In all honesty some good points here, we definitely do a variation of this - but I think the emphasis should probably be on show/ask - and mostly ask (two pairs of eyes are better than one).
> Show (open a pull request for review, but merge into mainline immediately)
Why merge into mainline immediately, doesn't their pipeline deploy on branchname.ci.example.com or something? Like in GitLab review deployments with k8s or other eXtreme DevOps practices.
Why merge into mainline immediately, doesn't their pipeline deploy on branchname.ci.example.com or something? Like in GitLab review deployments with k8s or other eXtreme DevOps practices.
"Show: open a pull request for review, but merge into mainline immediately"
This is literally the opposite of Shift Left. Merging in crap and talking about fixing it later. This should be renamed the "Tech debt accelerator".
This is literally the opposite of Shift Left. Merging in crap and talking about fixing it later. This should be renamed the "Tech debt accelerator".
I'm not following. We do reviews because a second pair of eyes can spot things the author missed.