GitHub allows us to lock down who gets to commit to branches, and one easy way of using that is to disallow commits straight into the main/master branch. Because that forces all code to be code-reviewed, so only approved changes will land, and that'll mean all the changes are safer… right?
No, I'm here to argue that the impulse to increase safety by enacting more technical limitations is actually wrong, because the problem of changes being risky is fundamentally a people problem, is often theoretical, and adding technical limits reduces a team's ability to experiment. At the end of the day heavy change management processes lead to worse business outcomes, and I'd like to explain why branch limits adds more weight than one might suspect.
There is a caveat: There can be valid reasons to protect a branch, e.g. for compliance or legal reasons, or you might have a contributor-centric workflow where trust on the committers are low (e.g. open-source). But for the majority of medium-sized projects I think it's worth considering the drawbacks of enacting these limits.
My first point: How serious is the problem actually? How often is main branch broken? In principle a broken main is a crucial signal as it is an inability to deliver value to customers, so it is important a team takes it seriously. If a team doesn't care to keep their main branch green then there is a fundamental culture problem, and it won't be solved with a branch protection feature. As a basic principle main must be green.
But, that said, do you actually have a problem if main breaks? What an odd thing to ask after saying "main must be green", but what if the broken build gets fixed a minute later? What if main is broken for just a few minutes per month in total? What if the team consistently fixes the failing build quickly, and work to prevent it from happening again? In this case maybe the team is trying out ways of working faster, and the rare broken main is them finding a weakness which they then fixed… I mean, you don’t learn to run without falling, so the rare broken main can actually be a positive sign that the team is experimenting with ways to work even faster.
If that's the case it would be harmful to enact branch limits.
At the end of the day everyone benefits from a flexible, simple change management process that's as free of hard restrictions as possible. For example,
- Consider a simple typo, why should there be any friction in landing such a fix? It'd be a huge waste of everyones time to use a pull-request workflow just to fix a typo! It should literally just be “commit & push”.
- Or maybe a team makes a change together, why shouldn't that change just land in main? Since the team wrote it together there are no advantages to requesting a review, it'd just be pointless ceremony.
- Or you might be on call and see a trivial error that just needs an obvious fix. If you have high confidence in the change why shouldn't it just land? I trust you to take care of the system and so I trust your judgement call, if you believe a main-commit is safe then go for it.
Now, of course if main does actually break too often then the team should have conversations around that (it'd be an obvious topic for retrospectives). If a team reflects on why they keep breaking main I'd be shocked if they don't identify several ways to enable safer and speedier ways of working. I've seen teams slide a bit into unsafe "forgetting to run tests before pushing to main"-territory causing a few trivial broken builds, and that's a laziness that's most easily remedied by being better professionals. I mean, running tests before committing is a pretty low bar! 😅
It's not all about practical drawbacks either, it's also worth highlighting that restricting main-commits sends a signal that developers can't be trusted. That is a strange and difficult dynamic to emit into a highly-paid group of professionals. I’ve seen plenty teams with no branch protections keep main fully green and I think it's much nicer to have that result come from a place of trust than by putting hard gates in the their way.
The ideal change management process is as lightweight as possible, fundamentally designed to create and nurture a high-trust environment. I mean, yes we want as much automation in there as possible, but we need to be careful not to add rules and restrictions that can pointlessly slow us down. Teams should be encouraged to continuously experiment and refine their ways of working, and tools and heavy processes should do their best to not get in the way of that.
Top comments (13)
While you make a compelling argument, I have to disagree with the notion that removing protections from the trunk is a feasible approach when collaborating as a team.
My reasoning is two-fold:
It’s a Quality control. Yes, you could just commit a typo fix to master quickly, but what if you introduce a new bug accidentally? Pull requests and peer review help build confidence in the changes being committed.
It’s not compatible with some engineering cultures. Our culture draws heavily from the principles laid out in the book Accelerate. Removing the trunk protection and allowing commits directly would begin to negatively affect performance metrics our organization maintains that ensure consistency, predictability, and stability of our releases and their cadence.
I’ve worked in collaborative teams ranging from 5 devs to 50, and in every instance we’ve kept branch protections to avoid accidents. It may be extra work to undergo the process, but it saves time in the end by avoiding unnecessary emergencies to unblock your release workflow.
That may be your experience, but I have the opposite. Forcing merge requests is:
The drawbacks in practice are numerous and the process mostly an annoying waste of time IMHO.
Please note that I'm not against merge requests themselves. I would just not force it. If it's a senior dev familiar with the code doing a small fix, let him do it directly. If it's a newcomer unsure of a large refactoring, of course let him do a merge request and review it. IMHO leaving both options on the table leads to a more productive outcome.
@dagnelies all your points are exactly how I feel too, nice to not be alone 😅 It's all about options for me too, I find tremendous power in keeping a process flexible.
I wanted to also add the option of post-merge requests, where I merge in a change without review and then later invite a colleague over to go through my change. That can also be a suitable way of working in some contexts.
While forcing merge requests might be annoying for you - I would question why?
The act of raising a review isn't problematic at all, and the act of logging in, reading and pressing a green button isn't problematic either. So what annoys you about it?
Speed (colleagues not being available) is fine, sometimes we should slow down.
If you experience merge conflicts, that tells me that you didn't pull the latest main into your branch before raising the PR. That's a quality concern all of it's own, you're not contributing to the HEAD of main.
Having fruitless discussions is a culture issue - our choice is that PRs exist (but are optional in some cases), and they should be merged if the code is functional. If there's a debate to be had, open a new tech debt ticket and we can talk about it in there.
If you think that tests will catch all bugs, and therefore tests should replace reviews - oh man, I have a bridge to sell you...
Ugency, is one point where we agree - a critical hotfix, Production on fire - should not go via a PR. There are other ways to review those.
I don't see how a PR breaks flow - as a developer, you raise the PR and move on to your next task. I bunch up PRs for reviews and do them while I'm drinking coffee in a morning (again, we should take the time to slow down, IMO).
My version history looks rather simple, with our optional PRs. Most work goes via PR, some does not. I guess the secret there, is in the commit messages.
I guess I just experienced the pain of our recent enterprise-wide policy of creating branches for each tiny bug/feature associated with MRs/reviews. It simply adds beraucratic burden and leads to all the outlined problems. The conflicts are also not there initially, they occur due to delayed reviews. Aslo, sometimes, you must wait until something is merged before you can "build upon it". It just slows down everything and feels unnecessary most of the time. Like I said, for complex stuff or unfamiliar stuff, MRs and reviews definitely make sense and should be done. On the other hand, blindly enforcing for each tiny detail is IMHO just an unnecessary burden. Due to all the merging happening accross the board, the version history is also a lot less linear, and if someone merged it wrongly, more difficult to fix.
I manage a small team, and we look after almost 60 different components.
All of your problems can be solved, if there's a wider desire in the company to change those situations.
For what it's worth - I probably agree with you - we do a hybrid approach of "whatever makes the most sense for the situation we're in" - sometimes that's a PR, sometimes it's a hotfix, sometimes I'll just hop directly into the Production system to tweak something.
I suggest you speak with your development management, and with your PMO, along the lines of "hey, this recent policy feels like it will hurt delivery timelines, has X, Y and Z been considered already?"
Re your #1 - if your concern is that another bug might creep in, then I would question if you trust people to check their work before they push a commit.
I personally use an IDE and a separate git GUI for that very reason.
And #2 is just a company decision, and could be changed, if wanted.
Hi Joe, valid points, and of course great to hear teams can weigh up the pros and cons and still find it worth having the branch protection.
Sounds like I've been in smaller teams than you (certainly not 50! 🤯😅), where main-commit accidents were rare and always short-lived so it never materialized as a tech-problem for us. I'd be a fan of trying an experiment to measure the with-and-without effect, if a team was curious to enable or disable branch protections
I would really love to read more about your performance metrics, it sounds like you've attempted to automate the DORA numbers? I've also experimented with that but didn't find a need to enforce pull-requests (TBH this feels like a topic worthy of a whole dedicated blogpost so it doesn't just disappear in the replies)
We use a hybrid approach.
Standard features, worked on in isolation, are done in feature branches, that are then subject to pull request & relevant review.
Emergency hotfixes, are always done by at least two people looking at the problem, so they have in effect been reviewed while being developed, and things are already on fire - so this is both time critical and we're not likely to make it worse - so they go direct to main.
But also, your seniority level (as a proxy for trust) defines what you can do - a junior that's only worked with us for a week won't be committing to main or bypassing anything. A senior developer (or above) can write direct to main, while management tiers can push things to Production without it even being in git.
If you work on any older or larger project then you will find yet another place where protecting main and using short-lived branches with pull / merge requests is absolutely necessary.
Under the above conditions, I have worked on several teams. With protected branches, a pull request requirement, and a pipeline with full end-to-end tests that is triggered when a PR is opened, I have seen the PR pipeline fail to pass quite regularly. That would otherwise be bugs in front of users. Protected branches insure the quality experience of our users.
It's not so much about security, its about CI/CD flow. Master is usually the branch that gets automatically build and deployed on a staging environment. You don't want to have bugs in staging if it can be avoided. Devs should always work on some form of a dev branch, but i agree we don't always need merge/pull requests on the dev branch. Pull requests are just convenient when working on features or bug fixes because it gives your team something tangible to review and its easier to revert than individual commits
There's a few problems with this, and all of them can be solved with a separate
stablebranch.It should be but only to development branches not the main or master. Most of the times we have deploy pipelines executing from these branches and if things go wrong its a mess.