12 ms·

Ask: A modern branching strategy

158 points by NicoJuicy 5y ago

pjc50 5y ago
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.
- whymarrh 5y ago
  Even inside in the web world, this approach wouldn't quite work for features with even a modicum of state.
  - wdfx 5y ago
    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.
    - zimpenfish 5y ago
      > 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.
      - telesilla 5y ago
        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!
        zimpenfish 5y ago
        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.
    - jeffreygoesto 5y ago
      Same here. In addition I also can see reviewing other people's code as education.
    - myk9001 5y ago
      > 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.
  - hvidgaard 5y ago
    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.
yoz-y 5y ago
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.
- capableweb 5y ago
  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.
  - JediWing 5y ago
    In theory yes. In practice, your test suites are not all-encompassing perfect oracles.
    - capableweb 5y ago
      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.
      - JediWing 5y ago
        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!
rossmohax 5y ago
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.
- gonzo41 5y ago
  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.
- rajamaka 5y ago
  You've never seen a blue/green deployment or a DNS change without downtime?
  - rossmohax 5y ago
    I've never seen _workflow_, were developer single handedly can create a well tested PR performing all necessary release steps upon merge.
    - rajamaka 5y ago
      At least for cloud environments, I haven't seen a company that doesn't have such a workflow in place for a long time.
      - rossmohax 5y ago
        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.
hibbelig 5y ago
> 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. I'm not following. We do reviews because a second pair of eyes can spot things the author missed.
- rossmohax 5y ago
  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.
  - ZephyrBlu 5y ago
    > 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.
    - heywherelogingo 5y ago
      I think you've read that the wrong way round.
    - ramraj07 5y ago
      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.
  - mumblemumble 5y ago
    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.
choeger 5y ago
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?
- jcon321 5y ago
  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.
- mmis1000 5y ago
  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.
tty7 5y ago
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
da39a3ee 5y 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.
jcon321 5y ago
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.)
- wodenokoto 5y ago
  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/ https://nvie.com/posts/a-successful-git-branching-model/
  - chrisweekly 5y ago
    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
- ramraj07 5y ago
  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.
  - jcon321 5y ago
    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.)
1337shadow 5y ago
> 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.
dd82 5y ago
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.
- MAGZine 5y ago
  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.
  - sgeisenh 5y ago
    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.
  - X6S1x6Okd1st 5y ago
    Have you never had a problem caught by a PR reviewer?
- alkonaut 5y ago
  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.
cestith 5y ago
Thanks. I hate it. As the saying goes, we all have a testing tier. Some of us also have a separate tier for production.
iainmerrick 5y ago
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.
- alkonaut 5y ago
  > 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.
travisgriggs 5y ago
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?
- iainmerrick 5y ago
  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.
cyb_ 5y ago
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/minimizing-review-lag-across-time-zones https://dev.chromium.org/developers/contributing-code/minimi...
- occz 5y ago
  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.
throwaway20371 5y ago
"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".
pierrebai 5y ago
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. Basically, freedom to choose is fragile. I think it is better to have an approach were Ship is exceptional, and pretty much everywhere I've work, there has been ways to avoid branching or reviews. I also think that the time-wasting vs time-saved due to reviews is heavily in favor of time-saved. We're no longer in the olden days, we got terabyte disk and easy branching. Starting to work on something else while waiting for a review is easy.
shagie 5y ago
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.
- alkonaut 5y ago
  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?".
e12e 5y ago
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).
riofoxx 5y ago
T.o a.s.s.i.s.t y.o.u i.n t.r.a.d.e.s f.o.r b.e.t.t.e.r i.m.p.r.o.v.e.m.e.n.t on c.r.y.p.t.o.c.u.r.r.e.n.c.y W.h.a.t.s.A.p.p (+13052395906)
corpMaverick 5y ago
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" - 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 More Specifically, most companies don't understand why this is important: "Everyone Commits To the Mainline Every Day". Also, there is too much branching going on.
- alkonaut 5y ago
  "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.
- da39a3ee 5y ago
  > 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.
  - corpMaverick 5y ago
    > 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.
- 5y ago
bguthrie 5y ago
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.
- peheje 5y ago
  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.
jkingsbery 5y ago
> 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.
zepolen 5y ago
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.
gocartStatue 5y ago
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 https://www.youtube.com/watch?v=v4Ijkq6Myfc

158 points by NicoJuicy 5y ago

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.

whymarrh 5y ago
Even inside in the web world, this approach wouldn't quite work for features with even a modicum of state.
- wdfx 5y ago
  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.
  - zimpenfish 5y ago
    > 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.
    - telesilla 5y ago
      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!
      - zimpenfish 5y ago
        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.
  - jeffreygoesto 5y ago
    Same here. In addition I also can see reviewing other people's code as education.
  - myk9001 5y ago
    > 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.
- hvidgaard 5y ago
  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.

yoz-y 5y ago

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.

capableweb 5y ago
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.
- JediWing 5y ago
  In theory yes. In practice, your test suites are not all-encompassing perfect oracles.
  - capableweb 5y ago
    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.
    - JediWing 5y ago
      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!

rossmohax 5y ago

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.

gonzo41 5y ago
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.
rajamaka 5y ago
You've never seen a blue/green deployment or a DNS change without downtime?
- rossmohax 5y ago
  I've never seen _workflow_, were developer single handedly can create a well tested PR performing all necessary release steps upon merge.
  - rajamaka 5y ago
    At least for cloud environments, I haven't seen a company that doesn't have such a workflow in place for a long time.
    - rossmohax 5y ago
      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.

hibbelig 5y ago

> 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. I'm not following. We do reviews because a second pair of eyes can spot things the author missed.

rossmohax 5y ago
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.
- ZephyrBlu 5y ago
  > 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.
  - heywherelogingo 5y ago
    I think you've read that the wrong way round.
  - ramraj07 5y ago
    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.
- mumblemumble 5y ago
  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.

choeger 5y ago

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?

jcon321 5y ago
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.
mmis1000 5y ago
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.

tty7 5y ago

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

da39a3ee 5y 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.

jcon321 5y ago

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.)

wodenokoto 5y ago
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/ https://nvie.com/posts/a-successful-git-branching-model/
- chrisweekly 5y ago
  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
ramraj07 5y ago
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.
- jcon321 5y ago
  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.)

1337shadow 5y ago

> 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.

dd82 5y ago

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.

MAGZine 5y ago
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.
- sgeisenh 5y ago
  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.
- X6S1x6Okd1st 5y ago
  Have you never had a problem caught by a PR reviewer?
alkonaut 5y ago
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.

cestith 5y ago

Thanks. I hate it. As the saying goes, we all have a testing tier. Some of us also have a separate tier for production.

iainmerrick 5y ago

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.

alkonaut 5y ago
> 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.

travisgriggs 5y ago

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?

iainmerrick 5y ago
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.

cyb_ 5y ago

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/minimizing-review-lag-across-time-zones https://dev.chromium.org/developers/contributing-code/minimi...

occz 5y ago
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.

throwaway20371 5y ago

"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".

pierrebai 5y ago

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. Basically, freedom to choose is fragile. I think it is better to have an approach were Ship is exceptional, and pretty much everywhere I've work, there has been ways to avoid branching or reviews. I also think that the time-wasting vs time-saved due to reviews is heavily in favor of time-saved. We're no longer in the olden days, we got terabyte disk and easy branching. Starting to work on something else while waiting for a review is easy.

shagie 5y ago

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.

alkonaut 5y ago
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?".

e12e 5y ago

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).

riofoxx 5y ago

T.o a.s.s.i.s.t y.o.u i.n t.r.a.d.e.s f.o.r b.e.t.t.e.r i.m.p.r.o.v.e.m.e.n.t on c.r.y.p.t.o.c.u.r.r.e.n.c.y W.h.a.t.s.A.p.p (+13052395906)

corpMaverick 5y ago

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" - 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 More Specifically, most companies don't understand why this is important: "Everyone Commits To the Mainline Every Day". Also, there is too much branching going on.

alkonaut 5y ago
"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.
da39a3ee 5y ago
> 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.
- corpMaverick 5y ago
  > 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.
5y ago

bguthrie 5y ago

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.

peheje 5y ago
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.

jkingsbery 5y ago

> 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.

zepolen 5y ago

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.

gocartStatue 5y ago

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 https://www.youtube.com/watch?v=v4Ijkq6Myfc