Why your team doesn't need to use pull requests
infrastructure-as-code.com
infrastructure-as-code.com
No, it's like having an editor proofread your article before you submit it. It's a second pair of eyes to make sure you haven't missed something or made a change that will subtly break things, or just misunderstood the requirements and implemented something not quite right, or didn't implement your tests properly and thus are not actually testing what you think you are.
CI isn't some magical fairy that ensures program correctness; it's only as good as the tests it runs. And even the best tests do not guarantee correctness.
I agree with a lot of points the author makes: there are other methods of increasing velocity and reviewing code that don't involve costly PRs. But they've become doctrine at many companies, even tiny startups that don't need a lot of process breaking velocity. Know the right time and context.
I think the main disconnect is that, decreasing velocity is the point of a PR in a way. We usually want to stop the dev process to have people look back at what's developped as a unit of code, and judge it with some distance, possibility from another team to create even more distance from the code.
That's where some teams put the checklists, or force a recheck of the requirements to validate the code actually matches what we want, etc.
So sure, if someone only cares about velocity PRs are just a burden, but that's far from a consensus IMHO (I'll show my hand by confessing I open PRs for myself, on my own private project, just to review the code in another context than my editor...)
> 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 out the window and it takes weeks to recover (unless you just lock out everyone until shit’s been fixed). It’s already hard enough when you just have flaky code, or time-sensitive tests.
If you want light reviews that’s a factor of development & review policy: let people self-review, make review scope clear, break up major changes into small tasks, …
Also I wish our CI took 10 min :( As of now it can take up to 90 minutes just to compile the codebase (although caching usually mean it's much faster than that.)
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.
> 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...
> 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.
You get your local copy into a healthy state - build passing, tidied up, refactored - and then you push it upstream. The idea is precisely that you only push healthy code, and so never break mainline.
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.
I've worked in trunk-based development shops for fifteen years. My colleagues have mostly been good, but certainly not superhuman. It works fine.
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.
> 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.
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 added check.
FWIW in a team of only experienced (>10 years) devs, I find problems in approximately 20% of the PRs I review (granted our codebase has numerous subtleties due to the esoteric nature of our domain). We've also had problems that even the reviewer missed; problems that broke mainline and made merging impossible until the issues were fixed.
It's fine to say "we'll just accept the added risk", but you really need to do it with eyes open. I personally won't accept such risks unless I absolutely have to (i.e. can't find a reviewer and the customer is breathing down our necks). That's one thing that 25+ years in the industry has taught me.
About a quarter of the article is about how to do code reviews in the context of trunk-based development.
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.
Pull requests with code reviews obviously make since in a large number of environments. But so does committing directly to a single dev branch for others.
> CI isn't some magical fairy that ensures program correctness
So very true, it much better to see it as a check for regressions or a small number of very well defined core areas. Proper user testing by someone who is trying to break it is so much more important.
Far too often small teams try to use tools and procedures that have been designed for large teams and businesses, when in reality they have been designed to solve organisational issues that small teams don't have.
> every line of code has to be vetted by at least 3 developers
Those are not equivalent. Reviewers typically don‘t "vet every line of code".
If you have a pair with 1 person with good knowdledge of the code digging deep into what the PR/MR does, and another dev just checking for anything surprising, or even just validating that what you're doing makes sense at the surface, that will probably cover most bases.
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.
That's not what I said, and is not my experience.
PR-s influence the same code, so it is not enough to review PR-s individually anyway. Maybe a weekly or monthly release would be a better unit for evaluation.
We have all kind of ways of producing diffs between branches, releases, tags, timestamps, etc? Why would PR diffs be the most efficient unit to review?
Often PR-s are just a silly way to communicate between team members instead of just talking to each other. I.e., you sit in the same office and create and reject PR-s, when you could just say to you colleague: "if you do not like the name of that variable, feel free to change the name".
The main problem with the PR system is that changes to the code is not immediately committed to the branches that developers work on, so it limits cooperation between developers. You can still review commits to dev branches and if problems are found, you can fix them or in most cases undo them.
Yeah, no. The challenge of running a company is that even within your company, everyone becomes misaligned the moment you’ve aligned. Have you hired anyone new? They don’t understand the company’s vision, nor have internalized your engineering standards. And even once they’ve worked with you for years, they still won’t have aligned on the things that they never ran into, AND they’ll misalign on all the things they've developed opinions on.
> Doing this means that a human only spends time reviewing code that has already been proven technically correct.
Yes, CI is useful for ensuring technical correctness. But technical correctness is one very small aspect of reviewing, and usually not where the big issues come in day to day.
Getting eyes on code is not about preempting things that would fail CI anyway. You probably should be running your full CI suite on PRs, too. To me it’s about catching misalignments on what the product should do, broader misalignments on how it should be engineered, catching misalignments that someone who’s too deep in the weeds would miss, talking through issues realized after the requirements had already been set and discussed, and more. A CI pipeline can’t fix that, pair programming will only address the things that an engineering peer who is as equally in the weeds as the contributor is, and integrating the newest things to production without human review is nice if your product is sufficiently low stakes that subjecting users to the effects of these people problems is okay; but that’s not for me and my team.
Pushing faster is not a pure good without trade offs.
I would argue that Pull Requests (or Merge Requests) are the single most efficient practice to ensure the quality of the code in a project with many people, right after the CI with linters, formatters, tests, etc. which you should set up even if you work alone (at least running them locally). Not to mention the knowledge sharing aspect of PR/MR reviews. It is a very versatile practice where a reviewer (or several reviewers!) can review the code itself from a different perspective and in a different setting, review the design (or "architecture", if you want), learn something new themselves, and keep up to date with the changes in the source code, but also in a product in general. Even in my solo projects I always take five before committing the code, in order to review it first with a clear head.
With that said, despite advocating at points for pair programming, mobbing, incremental reviews and having worked on a variety of different systems, and having success with some of them, I keep ending up back with PR's for the majority of places I've worked.
The reason why, I think, is one of cognitive load. Pull requests are easy. I don't think they're the best approach to code review ( big PR's often don't get reviewed effectively, and being an effective PR reviewer is a hard skill ) but, it's a good fallback. Everyone knows what a pull request is, the tooling for supporting them is excellent and it requires the least upfront work to get a code review.
With that said, I would highly encourage folk to look at different ways of reviewing code. I don't think many people will fully replace Pull Requests with a solely straight to master development approach utilising non-pr code review styles, but, having the alternatives in your box of tools for specific situations is really powerful.
However, I don't see pull requests going away because, if nothing else, they're an easy, low cognitive load approach for having a second set of eyes on some code.
I favour a less automated solution: code previews.
The biggest reason I've seen for code reviews taking a long time, is that they're surprising. If an engineer has implemented a full solution without communicating their intention ahead of time, reviewing it is really hard because the reviewer is often starting from zero.
1. Communicate your plan ahead of time
2. Check in with the team whenever meaningful progress has been made, and during/after key decision points (you can use draft PRs for this).
3. When it's ready to land, hopefully enough incremental reviews have been done that the final review is simple box ticking (and yes, perhaps it can be fully automated with CI).
Essentially, I think getting humans to look at your code is important, but it doesn't have to be done in a way that feels like a roadblock.
"pull requests sacrifice performance, including both delivery time and quality" Delivery time does not matter if your feature is not fully implemented; "Your team members should share your norms for quality" (paraphrased)
The article mentions having "regular, scheduled reviews" which sound like a chore and break flow more than having to review a PR every now and then. Having these on a weekly basis (as the article suggests) while having everyone push daily means you have 5 days of incomplete, unchecked code in your codebase, per engineer, every week.
CI and PRs are not mutually exclusive, you can have your github PR in "draft-mode" and it should still run the same workflows. Have your tests be automated and when the feature is done and passing; you request a review.
Pair programming.. sure.. but PP is so resource intensive and often it's better to have people focus by themselves imho.
With junior devs is too easy to become something in which the senior dominates the conversation.
Wholeheartedly disagree on the last part. Yes, Pair programming is expensive, but the knowledge transfer is invaluable. I would encourage people to work together on the same problem.
I always wonder if the pairing advocates and extroverts, and if that's why it hasn't really caught on in a field with a high percentage of introverts.
Even a couple of hours of pairing per day is a huge team productivity boost though. Don't do it "all day every day".
It's OK to say "I'll go away for the afternoon to flesh out these tests as we have discussed".
I'll say that it's less about if the PR exists or not, but by the time it's raised it should be fine to merge it pretty much as soon as it's green, because people are on the same page before the code was even written.
I do wish pairing advocates talked about this. It feels mildly fraudulent not to.
I'm not sure introversion vs extraversion is relevant. Firstly, i'm not sure they're even real things; they show up in a lot of snake oil psychology (Jung, Myers-Briggs), but i don't know to what extent they label a real axis of variation. But if extraversion means "the state of primarily obtaining gratification from outside oneself", then an extravert ought to hate pairing, because it's the same as programming solo, but with someone constantly pointing out what you've done wrong. The conversation you have during pairing is very, very different to a normal social conversation, because it's so focused on the work in hand.
Calling Jung snake-oil psych is a bit of a hot take!
Outside of that, Introversion/Extroversion is part of the Big 5 personality traits which I understand to be widely accepted and (unlike a lot of psych) replicated multiple times.
In any case, at the end of a day of pairing I find myself exhausted and have basically no desire to communicate with anyone for the rest of the day. Maybe that's something I'd get used to eventually, though I'm doubtful.
Maybe I work on very different code bases to the author, but having half-done work merged into `master` is a terrible idea. The sands are always shifting under your feet as a developer and it's almost impossible to successfully integrate manual testing into your process (testers need a stable base to test against). Yes, you could feature-switch, but it's easy to make a mess of things and it nullifies some of the "integrate often" benefits.
> No form of code review is more effective than pairing. Feedback is immediate, so there is a far higher chance you will use it to make improvements.
So pull requests are too costly, but pairing isn't? Pairing may reduce one class of errors, but it likely exacerbates others. If the reviewer is "too close" to the code being reviewed they'll miss many of the same things that a single developer would. Not to mention, it's exhausting for a lot of people (myself included).
PRs aren't magic, but there's a reason they've become so popular. And no, it's not just management liking "control" or something - they make the codebase better, which makes the developer experience better.
I also think pull requests in github, as an actual place you can go and comment on and discuss code, is such a useful tool for doing reviews in a public environment where everyone on a team can go back and refer to the discussions that I'd be inclined to accept some amount of delay as an acceptable cost.
- it’s extremely hard to correlate changes to comments, especially when rebase / force push are involved
- long discussions are extremely hard to follow as they’re not threaded, especially as “technical” events get mixed with people talking, you can easily have more noise from technical changes (tags, pushes, issues / PRs linking to the PR, review requests) than you have human comments, and GitHub’s forceful folding of the middle of the discussion makes things worse.
- inline comments quickly make diff views unreadable
- there’s no good / built-in handling of individual concerns to resolve, you can “resolve” inline annotations but you have to visit individual resolved threads and pray the author left some indication as to how they resolved the thing otherwise you’ve got no idea
Replacing pull requests with pair programming doesn’t make adding features to the main line necessarily faster. Review still happens, just synchronously, and you have two people working full time on the same thing, instead of one.
Personally, I have never worked in an environment where a few hours delay in reviewing a pull request, was such a big of a deal.
It also seems to disregard the fact that continuous testing can and does happen, because a feature branch may run against all the usual test fixtures, e.g. plan branches in Bamboo work for this.
I have introduced prs on low quality teams and guess what happened?
Higher quality, better learning, faster development because we do not constantly fix bugs in production.
Of course automate as much as possible, make the pr cycle short and explain people that creating the pr is not at 99% and you only wait for the button press but more like 70% and expect learning and small rework after.
Pull requests are really awesome. Slowing certain things down on purpose can help you go a lot faster.
The trick with merge requests is to keep them as small as possible while being something that can potentially be deployed into production, ie some kind of fix or feature, not just dead code.
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 oversimplified, but it's a qualified yes.
Continuous deployment is a very safe and productive way of working. It encourages small batches of change and thorough automated testing. If you find it "ludicrous" that says more about you than anything else. There are books on the subject if you're interested.
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).
Mainly for your colleague's stress levels, and secondarily for the business's success.
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.
Note that i'm not endorsing the practice. I find it scary too.
Because that’s what PRs are mostly: validation that they won’t break the build on main. Reviews aren’t strictly necessary but compiling is.
In some small project you might be able to run compile/test as part of a commit hook but for a situation where builds might take several hours or the developer can’t test all platforms that CI will, then it’s only possible to keep a green main by validation. In that scenario reviews wouldn’t be the limiting factor either.
This is very nearly touched on in the "pipeline approvals" section:
"...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."
That's a very good use case for a pull request with CI that builds/tests the result of the merge.
Depending on how long your builds are it doesn't even really take any extra time, if they're done in parallel
That doesn't actually stop you from breaking mainline though, just make it significantly less likely.
Say you are developing a class Foo, and I'm adding new code that uses Foo. You decide to refactor Foo to Bar. We won't get a conflict because I'm adding new code and not touching the Foo class.
In that case we both have pull requests that build fine but that once merged, won't compile. Running pipelines on each PR reduces the breakage but it can't eliminate it, unless you only allow one PR to build then merge at a time.
Git was designed around the idea of a clean history. Now, one might claim that "designed around the idea" does not necessarily mean that it doesn't work if you don't keep a clean history. However, there's a large amount of tooling within git which outright breaks if you don't stick to a clean history. As such, I am just going to go out and state that if you don't want to keep a clean history, you're at best not getting as much out of git as you reasonably could, and at worse, getting less out of git than if you were to use an alternative which was designed around the idea of not keeping a clean history (e.g. fossil).
In summary, even though I often see software projects already doing a bad job of maintaining a clean history already (which is partly because of pull requests being the wrong tool for small change sets (see kernel development for situations where pull requests make sense)), this approach is even worse.
Want continuous integration to work best? Split up the work into smaller parts which can be contributed as smaller individual units (whether you use PRs or some alternative system). Continuously pushing incomplete changes (even if you round them off so tests pass and nothing immediately breaks) is just not compatible with git.
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.
When those changes include non-reversible database migrations, for example, it is better not to integrate that code into "mainline" before you've settled on a specific implementation.
No. Slow is smooth, and smooth is fast.