Edit: Longer response below, but if requiring peer reviews triggers you to start looking for a new job, what does your dev workflow look like? Do you do peer reviews at all or only sometimes?
Admittedly, branch protection requiring peer review into master was something we started for SOC 2 compliance.
But, it’s actually great if implemented well.
Some suggestions:
- Limit the use of “master” branch to code currently deployed to production (don’t use master to stage code that hasn’t been deployed yet). This branch should have the strictest restrictions (no admin override) because merging anything into this branch only happens immediately before a deploy. This also allows your team to assume that any pushes to master should/could trigger an automated deploy workflow
- Have a release branch where all PRs are merged/squashed into. Use this branch as the base branch for all PRs. At our company this is simply the “staging” branch. This branch can have looser restrictions (allow overriding restrictions). The release branch is anything that’s going to be deployed in the next release. Merged into the release branch can trigger automatic deploys to staging for QA team to do any final functional testing before code gets to production
- Or for a smaller company, don’t have any restrictions on the release branch. You’ll still be in compliance with all frameworks because, at a minimum, no code gets to production without a peer review (because the entire release branch has to be peer approved before it can be merged to master)
- Still allow hotfix PRs into master if you need to deploy something urgently without deploying the release branch. But hotfixes ideally should be rare (if they happen all the time it means you’re deploying a lot of buggy code to production and probably need to do better testing/QA)
There’s always a balance between security/quality control and productivity.
At the absolute minimum, you really should enable branch restrictions even if the only restriction is requiring all merges come from a PR. This will block developers accidentally force pushing their local master and potentially overwriting master branch completely (this has happened at our company prior to enabling restrictions, and it required another developer force pushing their, more up to date, local master to resurrect the correct state)
The goal should be to block actions that are obviously bad in all cases (e.g. pushing a commit directly to master without a PR).
Whatever your branch restrictions are, they should align with whatever your company’s internal code review processes are. Then, the branch restrictions are simply acting as a fall back in case developers make a mistake (e.g. avoids merging/pushing to master by accident)
For a small startup, all this advice is irrelevant because you probably care a lot more about how fast you can pump out changes and care much less about bugs getting into prod. And that’s fine.
These suggestions are mostly relevant for mission critical code bases where the restrictions align with your QA process. The branch restrictions should be enabled after documenting a QA process. Branch restrictions should not be arbitrarily enabled if the reasons for the restrictions don’t align with your existing internal processes/workflows.