Why your team doesn't need to use pull requests

https://infrastructure-as-code.com/book/2021/01/02/pull-requests.html

by aard • 3 years ago

30 73 3 years ago

pringk02• 3 years ago

I get what is being said. There can be a delay in waiting for a review, but in my experience that's more an issue of bad discipline. I do get the point about pair programming making reviews near-redundant, though I'd be inclined to still get review from a third party.

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.

lopis2• 3 years ago

Agreed. It's part of my job to review my colleagues code, and I expect reviews from them ideally within a couple hours, maximum 24 hours. I find this practice also helps with knowledge sharing and breaking silos. And while I wait for a review, I can start another task, do some manual QA, grab a coffee, write some docs for this feature, etc. I rarely find myself stuck waiting for reviews.

masklinn4• 3 years ago

My biggest issue is really that github PRs are shit, and especially for large PRs they scale very badly (though that’s also an issue of having large PRs):

- 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

lodovic3• 3 years ago

I strongly disagree with this article. I wouldn't trust myself or my my coworkers to produce perfect code every time. We have 2 reviewers mandatory for each PR and using pull requests is a perfectly acceptable way to ensure quality - every line of code has to be vetted by at least 3 developers (it has to pass the CI/CD pipeline checks as well). If you produce high end software with complex workflows and/or calculations, pull requests are essential.

stinos• 3 years ago

I don't disagree with everything in there, The trouble with pull requests for instance is very real, but yeah, without mentioning the trouble with continuous integration nor the benefits of pull requests, it is just a pretty weak and one-sided article. Especially developers should now better than state 'B is bad because xxx and A is good because yyy so pick yyy'.

Hendrikto• 3 years ago

We have 2 reviewers mandatory for each PR
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".

yunohn• 3 years ago

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.

twic8• 3 years ago

The author does not say that you should not review code. There is a whole section of the post about how to review code.

nelgaard• 3 years ago

But is it the most efficient way to ensure that level of quality?

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.

lopis2• 3 years ago

In our setup, the committer can merge their pull request after CI passed and at least one review has approved the code. Seems like a better compromise than relying solely on CI. And pairing is nice, but there's not always someone available to pair with. Heck, I always review my own pull requests completely before requesting a review, because I often miss something. And the easiest way to do this, that I know of, is with pull requests.

kstenerud3• 3 years ago

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.

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.

emptysongglass• 3 years ago

The article actually touches on other methods of review later on.

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.

makeitdouble2• 3 years ago

His alternatives to PRs are pair programming and after the fact reviews (when the code is already in the mainline). I don't see any orgs where we would have wanted that, small or big team.

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

masklinn4• 3 years ago

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 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, …

xmcqdpt22• 3 years ago

Yes. I work on a monorepo with >1000 other devs, in every time zone. If we just broke mainline all the time, you'd get people merging broken code in Asia at the end of their work day and then we would have to revert it in NA. That would be a nightmare!

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

JenrHywy4• 3 years ago

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.

twic8• 3 years ago

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.

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.

samwillis• 3 years ago

Exactly, but it's also important to note that different teams have different requirements, and the QA on those will be different for all.

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.

devnullbrain• 3 years ago

Yes, (competent) code review is constructive, not gladiatorial.

jacknews2• 3 years ago

Is the author suggesting every commit goes into the main branch, and into production? That seems ludicrous.

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.

Arch-TK2• 3 years ago

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

SideburnsOfDoom5• 3 years ago

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

bdsa• 3 years ago

In some domains that is an explicit goal.

jacknews2• 3 years ago

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.

twic8• 3 years ago

This is called "continuous deployment", and it was all the rage ten years ago. I'm surprised the idea seems unfamiliar now, but that's how this industry works, i suppose!

ocius• 3 years ago

Some good points! There are, however, also changes that you do not want to propagate to your teammates or production unless you are 100% sure that they are the final solution to the problem you are trying to solve. Sometimes, the complexity of a problem is so high that you have to experiment with the code.

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.

Arch-TK2• 3 years ago

Strange advice.

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.

orng• 3 years ago

Where I work we have two code bases. The older of the two in TFS and the newer in git. In TFS we use trunk-based development and in git we do pull requests. I must say that I very much prefer having a formal, tool-protected, review process, doing integrations and testing on a separate branch from the main branch to allowing developers like we do in git to the added ease of being available to commit directly to trunk. Even with CI to run tests upon committed code you never want to leave your mainline branch in a broken state. No matter how fast the feedback is, because you don't really know how fast your developers will be to act on that feedback!

mmriis• 3 years ago

For us this wouldn't work. PR's are a great way of giving feedback and teaching junior devs. It also gives people people an idea of whats going on with the codebase and provides a way to align code style.

Pair programming.. sure.. but PP is so resource intensive and often it's better to have people focus by themselves imho.

MiyamotoAkira3• 3 years ago

I've used quite extensively Pair Programming, and at its best is better when both people tend to be senior, as you can bounce design ideas out of each other much easier, and is more likely that a bad decision (or test, or ...) gets caught earlier.

With junior devs is too easy to become something in which the senior dominates the conversation.

MrGilbert• 3 years ago

Pair programming.. sure.. but PP is so resource intensive and often it's better to have people focus by themselves imho.

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.

JenrHywy4• 3 years ago

I've done pairing in the past, and as much as it was fun and felt productive, it was completely exhausting. I don't know how people do it day-in-day-out.

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.

presentation• 3 years ago

An outsider may not understand the vision and direction of your project. They may not have the same habits and norms for testing, code quality, and style. However, your own team members should share these norms.

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.

kilmanio• 3 years ago

A pull-request based approach is more sane than changing CI/CD to CI/C-MergeConflicts

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

andrewingram• 3 years ago

I think it's good to challenge received best practices like this - even if in this case the alternative seems to hinge on something that most teams struggle with even more -- writing sufficient good tests.

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.

triggerity• 3 years ago

It's written to trigger me right?

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.

MiyamotoAkira3• 3 years ago

Going from nothing to some quality control is always an improvement. The point being made is that is not as good as other options.

MiyamotoAkira3• 3 years ago

After having used both PRs and Pair programming with a strong CI setup I found the later much better for quality, design and efficiency.

bob1029• 3 years ago

We work in a regulated industry and having a 2nd party who is required to review code prior to merging is essentially mandatory. We can produce high-quality logs and provide them for inspection. Every single line of code in our codebase has had at least 2 humans review it.

Pull requests are really awesome. Slowing certain things down on purpose can help you go a lot faster.

donatj• 3 years ago

For us, pull requests are VERY important. They are a chance for people with domain knowledge of other areas of the product to raise concerns.

obowersa• 3 years ago

Over the years I've experimented with getting rid of pull requests quite a few times. As a whole and solely as an abstract idea I wholeheartedly agree with the article. Code Review is not the same as a pull request.

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.

edaemon• 3 years ago

Hm, the section on using CI rather than pull requests seems odd. PRs can be part of CI. You can set up your pipeline to build and test the result of a PR; that is, you are testing each change as if you had pushed it to the project's mainline, but there's no potential for you to break the mainline branch in a way that hinders your team.

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.

jaggederest• 3 years ago

I usually have two jobs on every PR: one that builds "as presented", and one that builds after an automatic integration with the parent branch. If the merge fails, you still have test feedback from the "as presented" build.

Depending on how long your builds are it doesn't even really take any extra time, if they're done in parallel

xmcqdpt22• 3 years ago

Hm, the section on using CI rather than pull requests seems odd. PRs can be part of CI. You can set up your pipeline to build and test the result of a PR; that is, you are testing each change as if you had pushed it to the project's mainline, but there's no potential for you to break the mainline branch in a way that hinders your team.

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.

ht85• 3 years ago

No wonder the blog author is a consultant. When you're not working on "your" project, blaming production issues on processes is always an option.

alentred• 3 years ago

I respectfully disagree. I worked with "trunk"s before, won't do that again, sorry.

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.

alexmorley• 3 years ago

Couple of other good articles about async code review linked here: https://news.ycombinator.com/item?id=36029328

i386• 3 years ago

Can we all just agree that the author is incorrect. We do not need to be second guessing fundamentals like code review and keeping mainline branch working. The goal of making software is to make working, profitable software. This person has lost perspective.

JenrHywy4• 3 years ago

“As they work on it” is essential. 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 integrate it into the mainline with everyone else’s current work.

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.

manuelabeledo• 3 years ago

I fail to see the real world benefit of this approach.

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.

Vanit• 3 years ago

However, pull requests sacrifice performance, including both delivery time and quality.

No. Slow is smooth, and smooth is fast.

anchochilis• 3 years ago

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.

alkonaut• 3 years ago

A first requirement for direct-to-main development is that you can be reasonably sure your code will not break the CI build otherwise you'll ensure perpetually broken builds and a process many times slower than PR:s.

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.

Why your team doesn't need to use pull requests

Related Stories