[HN Gopher] Ship / Show / Ask: A modern branching strategy
___________________________________________________________________
Ship / Show / Ask: A modern branching strategy
Author : NicoJuicy
Score : 144 points
Date : 2021-09-13 11:41 UTC (11 hours ago)
(HTM) web link (martinfowler.com)
(TXT) w3m dump (martinfowler.com)
| cestith wrote:
| Thanks. I hate it.
|
| As the saying goes, we all have a testing tier. Some of us also
| have a separate tier for production.
| corpMaverick wrote:
| 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 wrote:
| "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.
| jcon321 wrote:
| 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.)
| nahname wrote:
| Environments are generally handled via deployment
| configuration.
| wodenokoto wrote:
| 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/
| chrisweekly wrote:
| 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
| ramraj07 wrote:
| 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 wrote:
| 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.)
| ramchip wrote:
| 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.
| ramchip wrote:
| 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.
| iainmerrick wrote:
| 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 wrote:
| > 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.
| iainmerrick wrote:
| 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.
| cyb_ wrote:
| 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...
| occz wrote:
| 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.
| pierrebai wrote:
| 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.
| riofoxx wrote:
| 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)
| throwaway20371 wrote:
| "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".
| e12e wrote:
| 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).
| travisgriggs wrote:
| 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 wrote:
| 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.
| yoz-y wrote:
| 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 wrote:
| 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 wrote:
| _In theory_ yes. In practice, your test suites are not all-
| encompassing perfect oracles.
| capableweb wrote:
| 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 wrote:
| 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 wrote:
| 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.
| throwaway20371 wrote:
| 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.
| gonzo41 wrote:
| 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 wrote:
| You've never seen a blue/green deployment or a DNS change
| without downtime?
| rossmohax wrote:
| I've never seen _workflow_, were developer single handedly
| can create a well tested PR performing all necessary release
| steps upon merge.
| rajamaka wrote:
| At least for cloud environments, I haven't seen a company
| that doesn't have such a workflow in place for a long time.
| hibbelig wrote:
| > _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.
| spookthesunset wrote:
| Not just to spot issues but to make sure other developers on
| the team understand what else is being worked on. Spread the
| knowledge.
| tty7 wrote:
| 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.
| colourgarden wrote:
| Reviews are also a good way to learn. Particularly seniors
| reviewing the code of juniors and providing feedback (code
| suggestions are great for this).
| mumblemumble wrote:
| 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.
| mellavora wrote:
| 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?
| Jenk wrote:
| 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.
| lowercased wrote:
| 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...
| mixmastamyk wrote:
| Contractors are generally paid by the hour. So while
| frustrating, there's always the paycheck to look forward
| to.
| dopidopHN wrote:
| I think I left your previous work place a few month ago.
| It's just too similar. Each point.
| lowercased wrote:
| was the company in the southeast US? :)
| mixmastamyk wrote:
| 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.
| nitrogen wrote:
| 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.
| cratermoon wrote:
| > "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.
| Jenk wrote:
| 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.
| cratermoon wrote:
| 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...
| Jenk wrote:
| 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
| cratermoon wrote:
| 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.
| pierrebai wrote:
| 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.
| jt2190 wrote:
| 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.
| ljm wrote:
| 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.
| mukundesh wrote:
| I can't see how you can get any non-tribal code in the codebase
| without asking.
| SkyPuncher wrote:
| > 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.
| dopidopHN wrote:
| In my language the military has a saying << trust to not
| exclude control >>
|
| Simple and to the point.
| faizshah wrote:
| 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.
| davio wrote:
| Change Control has historically been "Change Prevention" in
| many organizations. They optimized for stability
| aaaaaaaaaaab wrote:
| 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.
| blacktriangle wrote:
| 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.
| christophilus wrote:
| 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.
| rossmohax wrote:
| 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 wrote:
| > _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 wrote:
| I think you've read that the wrong way round.
| ramraj07 wrote:
| 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 wrote:
| 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.
| JediWing wrote:
| 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.
| hvidgaard wrote:
| 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.
| eCa wrote:
| 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.
| mukundesh wrote:
| if code reviews come with responsibility sharing then the
| are going to slow things further, 100% agree with the rest.
| city41 wrote:
| 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.
| mumblemumble wrote:
| 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.
| refenestrator wrote:
| 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".
| peakaboo wrote:
| 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.
| frizzle112 wrote:
| 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?
| refenestrator wrote:
| 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.
| dv_dt wrote:
| 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.
| refenestrator wrote:
| +1 to taking it offline when it's more than a couple
| minor style fixes.
| notJim wrote:
| 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.
| mixmastamyk wrote:
| 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.
| refenestrator wrote:
| 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.
| icedchai wrote:
| 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.
| mixmastamyk wrote:
| 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. ;-)
| theelous3 wrote:
| 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.
| hoseja wrote:
| Yes! Too bad people rarely bother, nobody seems to have the
| time.
| withinboredom wrote:
| 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.
| necubi wrote:
| In many companies changes must be reviewed for compliance
| reasons (e.g., SOC2 will generally require some sort of
| review procedure).
| eyelidlessness wrote:
| 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.
| JediWing wrote:
| 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"
| cratermoon wrote:
| > 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.
| withinboredom wrote:
| 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.
| JediWing wrote:
| 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.
| MAGZine wrote:
| 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.
| cratermoon wrote:
| > 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.
| cj wrote:
| 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.
| reedlaw wrote:
| 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.
| JamesSwift wrote:
| 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.
| leftnode wrote:
| It also helps spread knowledge around so more than one person
| has knowledge of a new feature, bug fix, or sub-system.
| phendrenad2 wrote:
| 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.
| astonex wrote:
| If the change is extremely trivial then so is reviewing that
| change. Not much time is lost
| davidrupp wrote:
| 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,
| ...).
| munificent wrote:
| 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.
| drewcoo wrote:
| It's not about the time to review. It's about yet another
| interruption. For the sake of something trivial.
| alkonaut wrote:
| 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.
| dd82 wrote:
| 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.
| alkonaut wrote:
| 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.
| MAGZine wrote:
| 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.
| X6S1x6Okd1st wrote:
| Have you never had a problem caught by a PR reviewer?
| sgeisenh wrote:
| 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.
| bguthrie wrote:
| 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 wrote:
| 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.
| zepolen wrote:
| 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.
| shagie wrote:
| 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 wrote:
| 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?".
| jkingsbery wrote:
| > 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.
| choeger wrote:
| 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 wrote:
| 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.
| 1337shadow wrote:
| > 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.
| tty7 wrote:
| 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 wrote:
| 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.
| pjc50 wrote:
| 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 wrote:
| Even inside in the web world, this approach wouldn't quite work
| for features with even a modicum of state.
| wdfx wrote:
| 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.
| jeffreygoesto wrote:
| Same here. In addition I also can see reviewing other
| people's code as education.
| zimpenfish wrote:
| > 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 wrote:
| 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 wrote:
| 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.
| myk9001 wrote:
| > 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 wrote:
| 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.
___________________________________________________________________
(page generated 2021-09-13 23:01 UTC)