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 b...
For further actions, you may consider blocking this person and/or reporting abuse
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.