RFC: Protect main/master branches

[admbtlr]

Proposal:

Protect main/master branches on all repos

Reasoning

It's pretty much always (blurry language alert) a bad idea to commit directly to the main/master branch without having a second pair of eyes on your code/docs. Sometimes it happens by accident; sometimes it happens in a rush when it's perceived that there isn't enough time to do the right thing; sometimes it even happens because someone wants to sneak in a change without anyone else noticing. Protecting the main/master branch using Github's tools makes it easy to avoid all of these from happening.

Exceptions:

I honestly don't think there should be any exceptions to this, and I have never encountered a situation where a protected branch caused more problems than it solved - except in cases where the protection was set in a configuration that was unnecessarily restrictive.

Additional Context:

Github makes it easy to stop people from committing directly to a branch, and instead require that all changes to that branch happen via pull requests. It then allows you to specify particular bars that those PRs must clear in order to be mergeable (tests passing, how many and which people approve).

I think that a sensible set of bars is:

• tests pass
• at least one person approves the PR

One restriction that I particularly like (but that some people might find too constraining?) is Dismiss stale pull request approvals when new commits are pushed. This stops people from making and then merging unreviewed changes in an already approved PR.

In most cases, everyone in the engineering org has admin permissions on our repos. This means that anyone can makes changes to these branch protection settings (within a repo, go to Settings > Branches > Branch Protection Rules).

How is this RFC resolved?

Two options, to soft way and the hard way:

The Soft Way

We agree that all Artsy repos should have main/master branches protected by default. Engineers can then change the settings to repos opportunistically whenever they come across unprotected main/master branches.

The Hard Way

We drop everything and turn on protection on every active repo.

I prefer the soft option.

[jonallured]

This seems good - no dissent from me! I do wonder if there's a middle ground between the soft and hard ways where we pick some repos and enable this there. Off the top of my head I'd go with:

• gravity
• force
• eigen
• metaphysics
• volt

I think if we do it to those projects it's possible the practice will radiate out from there. 🤷

[admbtlr]

Thanks for the list @jonallured, I enabled master/main branch protection on all of them

[admbtlr]

Resolution

We decided to do it.

Level of Support

2: Positive feedback.

Next Steps

I've protected main/master branches on the five main repos listed above. From now on, everyone should opportunistically protect main/master branches on any repo they work on, preferably using settings similar to those used on the above listed repos.

Exceptions

None, currently.

[mzikherman]

I'd like to resurface some issues with this now that's in practice.

First layer of protection- this prevents direct commits to the main branch. This seems great! No more sneaky pushes to master (or the more likely accidental one). I can see this saving someone from doing this inadvertently.

Second layer of protection- also requiring that all PR's must have a passing reviewer. This opens up a can of worms that I think this RFC may not have realized: what's the difference between assignee and reviewer? How does requiring a reviewer play with Peril, which already requires an assignee? How does this interface with 'merge on green'?

I sort of think that everyone was on board with the first layer, but the second layer feels a tad iffier to me. It's kind of like the branch protection rule was maybe the main 'goal' of the RFC and the review approval requirement maybe a bit secondary to that? Wonder if we should roll that back or make new one for it.

[admbtlr]

Yes, you're absolutely right, and I apologise for getting carried away when I changed the branch protection settings. I've turned off the Require approvals setting on Gravity and Volt (the only of the five repos that still had it enabled).