Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Why your team doesn't need to use pull requests

infrastructure-as-code.com

Post reply on HN
Print view View original

61–70 of 76 posts

Re: Why your team doesn't need to use pull requests

#61

Post by twic »

jacknews wrote: post #42
Arch-TK wrote: post #13

Earlier quoted context omitted.

Does a change being accepted into master equal it becoming live in production at your company? That seems even more ludicrous.

As others have said, not everything going into main goes straight to production, but it's the start of the process that should see it deployed. Nothing should go there unless it's a candidate for deployment. To me continuous deployment means deploying features as they are ready (and the smaller the better), rather than batching up big 'releases'. It does not mean deploying code line-by-line. All IMHO of course.

The term "continuous deployment" was absolutely coined to describe automatically deploying the trunk into production after it passes some set of tests. That's a sufficiently scary idea that that for many people the meaning has become watered down over time.

Note that i'm not endorsing the practice. I find it scary too.

Re: Why your team doesn't need to use pull requests

#62

Post by twic »

masklinn wrote: post #56
twic wrote: post #53

Earlier quoted context omitted.

No. You and those replying to you have completely misunderstood what the author wrote. My emphasis and a clarifying adverb: > As a team member, you don’t wait until you have finished a feature or story to integrate your code to the mainline. Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and [then] integrate it into the mainline with everyone else’s current work.…

No, we understand perfectly well what the author wrote, but we’ve also worked with actual breathing humans rather than just spherical cows in vacuum, so we understand what will inevitably happen. This is about as realistic as telling devs not to push bugs. As soon as it gets in the way or there’s a boss breathing down the dev’s neck, anything which is not enforced is jettisoned.

As far as i can tell from your comment ("as far as I can tell OP’s recommendation is to break the mainline"), you did not in fact understand what the author wrote.

I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.

Re: Why your team doesn't need to use pull requests

#63

Post by twic »

majewsky wrote: post #57
twic wrote: post #53

Earlier quoted context omitted.

No. You and those replying to you have completely misunderstood what the author wrote. My emphasis and a clarifying adverb: > As a team member, you don’t wait until you have finished a feature or story to integrate your code to the mainline. Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and [then] integrate it into the mainline with everyone else’s current work.…

Yes, and this is how things were supposed to go before PRs. Whenever PR requirements are added, it's always because the described process does not work in reality and mainline ends up filling with crap as developers under pressure try to rush their prio-1 features out the door.

That is not correct. As the author says:

> This practice has become so common that many people consider it a default, “best” practice. Some people assume there is no other way to make sure code is reviewed because they’ve never seen anything else.

And you can see this in the comments on this post: a few are "we did trunk based development and it didn't work, so we switched to pull requests", but most are "we do pull requests, and i can't see how trunk-based development could possibly work", so not reporting a switch based on experience.

And (as you may have seen me mention, sorry), i have been doing TBD for years, and have mostly not had a problem with the trunk filling up with crap.

Re: Why your team doesn't need to use pull requests

#64

Post by twic »

kstenerud wrote: post #59
twic wrote: post #53

Earlier quoted context omitted.

No. You and those replying to you have completely misunderstood what the author wrote. My emphasis and a clarifying adverb: > As a team member, you don’t wait until you have finished a feature or story to integrate your code to the mainline. Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and [then] integrate it into the mainline with everyone else’s current work.…

So basically everything that one would normally finish with a PR, but without a PR. In my daily work, I never reject a PR for failing tests (the CI system disallows merging until all tests, lint, warnings, danger, style have passed). So whenever I find a problem in a PR, it's due to something that is not detected (or possibly not even detectable) by the automated checks. By dropping the PR requirement, you lose that…

> By dropping the PR requirement, you lose that added check.

About a quarter of the article is about how to do code reviews in the context of trunk-based development.

Re: Why your team doesn't need to use pull requests

#65

Post by masklinn »

twic wrote: post #62
masklinn wrote: post #56

Earlier quoted context omitted.

No, we understand perfectly well what the author wrote, but we’ve also worked with actual breathing humans rather than just spherical cows in vacuum, so we understand what will inevitably happen. This is about as realistic as telling devs not to push bugs. As soon as it gets in the way or there’s a boss breathing down the dev’s neck, anything which is not enforced is jettisoned.

As far as i can tell from your comment ("as far as I can tell OP’s recommendation is to break the mainline"), you did not in fact understand what the author wrote. I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.

> As far as i can tell from your comment you did not in fact understand what the author wrote.

I like how you keep calling me a liar, it’s nice.

> I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.

The “E” in IME stands for Experience.

I’ve worked in trunk-based development shops for years as well, it did not work fine, my comments are experience from seeing it break down as the number of devs increased, and the time wasted on mainline being broken kept ticking up.

I’m happy for you if you never suffered from that, but I’m not going to argue for practices I’ve seen fail over and over.

Re: Why your team doesn't need to use pull requests

#66

Post by kstenerud »

twic wrote: post #64
kstenerud wrote: post #59

Earlier quoted context omitted.

So basically everything that one would normally finish with a PR, but without a PR. In my daily work, I never reject a PR for failing tests (the CI system disallows merging until all tests, lint, warnings, danger, style have passed). So whenever I find a problem in a PR, it's due to something that is not detected (or possibly not even detectable) by the automated checks. By dropping the PR requirement, you lose that…

> By dropping the PR requirement, you lose that added check. About a quarter of the article is about how to do code reviews in the context of trunk-based development.

Yes, but they are unconvincing. Pair programming is only acceptable to some developers (others it drives insane). It also induces tunnel vision because now both developers are too close to the code to see issues that an outside observer will pick up quickly. They're also intimately familiar with the structure and algorithms, and so they won't see things that someone fresh would have trouble grasping (meaning that they need to either simplify, name with intent, or comment appropriately). I actually haven't noticed too much of a difference in issues picked up on pair programmed vs solo programmed PRs; they tend to match the quality of the stronger developer.

I've done periodic code reviews at three separate companies. Unless I've been very unlucky, it generally goes like this:

* Everyone gets pulled away from their work for an hour and goes into a room.

* Someone has picked some code to review (either they've been forced into it, or they have something cool they want to show off). Mostly they're just terrified at the prospect of being humiliated in front of everyone.

* Everyone pretends to be interested, but they're really just waiting for it all to end as the CO2 levels slowly climb.

* A few minor details are talked about, and then someone asks why approach Y wasn't used. Relieved that there's finally a discussion point, everyone talks about this architectural idea for the rest of the hour.

* Due to time constraints, 90% of the code is never reviewed.

I spend about an hour a day on average reviewing PRs, and that's with a team of 5 engineers. If we reviewed all code in team meetings, that would multiply review time by 5x because now everyone is reviewing all code in series rather than in parallel.

Then there's this:

> Pipeline approvals: If your team uses a Continuous Delivery pipeline to deliver changes to production, you can include a stage that requires someone to authorize the change to progress. This is conceptually similar to a pull request in that it is a gate in the delivery process, but you place the gate after code integration and automated tests. Doing this means that a human only spends time reviewing code that has already been proven technically correct.

Except it hasn't been proven technically correct. It's just code that has been (maybe) pair programmed, and (maybe) discussed if it happened to be part of the 10% of the code looked at and discussed for architecture in the review meeting.

Re: Why your team doesn't need to use pull requests

#67

Post by clawlor »

JenrHywy wrote: post #46
masklinn wrote: post #25

Earlier quoted context omitted.

Also as far as I can tell OP’s recommendation is to break the mainline, then tell everyone it’s broken. > Instead, you frequently - at least once a day - put your code into a healthy state that passes tests and integrate it into the mainline IME that’s a perfect recipe for getting a mainline which never passes tests in any organisation of a non-trivial size, as soon as broken code has been pushed “giving a fuck” goes…

100% The author also seems to have missed that you can still use CI on feature branches. Merge `master` to your feature branch regular, run the CI. Sure, you're only integrating with other complete-ish work, but it gets you a long way without making a complete mess of things.

Some CI systems do this automatically for PR builds, e.g. TravisCI:

> Rather than build the commits that have been pushed to the branch the pull request is from, we build the merge between the source branch and the upstream branch.

https://docs.travis-ci.com/user/pull-requests/#how-pull-requ...

Re: Why your team doesn't need to use pull requests

#68

Post by SideburnsOfDoom »

lodovic wrote: post #50
yunohn wrote: post #33

Earlier quoted context omitted.

2 reviewers seems like overkill unless it’s they’re also testing actual working. Reading code in a PR is not worth so many people-hours.

Unless you work in a very complex technical domain with high profit margins. A bug in production can literally cost us billions (apart from potential reputation damage), the system is highly complex, so having 2 reviewers isn't a luxury for us but a necessity.

You could make the same argument about automation not manual process.

e.g. "A bug in production can literally cost us billions (apart from potential reputation damage), the system is highly complex, so having robust test suites, automated monitoring and rollback isn't a luxury for us but a necessity."

IMHO, automation wins every time. That is, while there is value in "stop the pipeline versions of code review", we value the automation more.

Re: Why your team doesn't need to use pull requests

#69

Post by anchochilis »

This blog post reads like it's written by someone using Subversion back in 2015. Most of the justification for committing on mainline can be addressed by following other best practices:

1. Run the same suite tests additional quality checks (linters, etc) on PR branches that you run on mainline

2. Run branch CI against the merge commit for your PR branch (i.e. include your changes as well as latest mainline)

3. Avoid long-lived branches.

If you run into frequent merge conflicts and incompatible changes in an area of your codebase such that your team needs to integrate individual commits every half-hour instead of keeping 1-4 day feature branches around... That sounds like a code and/or organizational smell.

However!

> Using pull requests for code changes by your own team members is like having your family members go through an airport security checkpoint to enter your home. It’s a costly solution to a different problem.

I actually do like this analogy -- but it's not PRs that are the problem, it's mandatory reviews. And the cost/benefit of mandatory reviews varies widely depending on how senior the engineers on the team are, how much their work overlaps, and what their average tenure on the team is.

Re: Why your team doesn't need to use pull requests

#70

Post by ruh-roh »

SideburnsOfDoom wrote: post #29
Arch-TK wrote: post #13

Earlier quoted context omitted.

Does a change being accepted into master equal it becoming live in production at your company? That seems even more ludicrous.

> Does a change being accepted into master equal it becoming live in production at your company? Where I am, and at other good companies that I know of, "a change being accepted into main" equals processes being automatically started, that progress it through further environments and test suites, that if they all go well, result in "it becoming live in production" with no further human intervention. So you oversimpli…

I joined a company last year that has a handful of legacy products, and we successfully moved one of them to a CD model a few months ago.

It's hilarious to sit back and watch every other team have the same monthly loop of big change -> lengthy test cycle -> "go-live" meeting -> fixes/big change -> more testing... etc. Loads of stress and finger-pointing.

Hardly ever hear a peep from the CD team. Sure, it's not all rainbows and lollipops. They do have urgent production issues time-to-time, but most are resolved by a rollback (or feature toggle flipped).

Post reply on HN