Browser-compat-data: Turn on branch protection rules for master

[ddbeck]

On #6615, there was discussion about what GOVERNANCE.md says about pushing to branches on this repo and also my fear of accidentally pushing to master.

@sideshowbarker suggested turning on branch protection rules for master in https://github.com/mdn/browser-compat-data/pull/6615#issuecomment-718039025. I think this is a great idea. In addition to curing my fear, it will also encourage some better bookkeeping around the small number of changes pushed to master directly, such as incrementing the package version number.

Tasks:

• [x] Review and, if applicable, revise names of required checks
• [ ] Enable branch protection rules for master
• [ ] Optional: see if we can protect any any branch with some sort of user or issue prefix. That is, branches _not_ in the form [\w_-]+\/.+ (e.g., someuser/topic or 1234/fixes-issue).
• [ ] Optional: retire any unprefixed branches
• [ ] Revise package publish doc to use PRs instead of pushing to master
• [ ] Revise GOVERNANCE.md to reflect our actual use of branches on the repo, rather than a never-practiced prohibition against using branches

[foolip]

For what it's worth, I found the prohibition against pushing branches to this repo to be refreshing, even if unusual. The branches in https://github.com/web-platform-tests/wpt just keep piling up, and that just can't happen if you never create them in the first place. There's also less noise when doing git fetch, which I appreciate.

[sideshowbarker]

For what it's worth, I found the prohibition against pushing branches to this repo to be refreshing, even if unusual. The branches in https://github.com/web-platform-tests/wpt just keep piling up, and that just can't happen if you never create them in the first place. There's also less noise when doing git fetch, which I appreciate.

As a counter-example, for WHATWG specs, we push branches to the canonical repos, and the branches don’t pile up.

But anyway, I have no objections to us enforcing a policy of prohibiting pushing of branches to this repo.

However, ideally we should be able to have GitHub actually prevent that. Does GitHub provide an option for it?

The only thing I am aware of that would maybe work to enforce it is if we configured the repo to restrict pushing to any branches — by specifying a wildcard in the pattern field of the branch-protection UI. But I’ve never tried that, so I’m not certain it actually would have the effect we need.

Anyway, the reason I personally prefer to be able to push branches to the same repo where they are getting merged (rather than to my fork) is that, once the PR does actually get merged, GitHub automatically deletes the branch. But if the branch lives in my fork, then I don’t know of any way to have GitHub automatically delete the branch when the PR gets merged.

So when my PR branches live in my fork, I always need to remember to manually push the button in the PR page to make GitHub delete the branch. Which is of course not a big deal — I just mention that as one minor downside.

[foolip]

I screwed up in https://github.com/mdn/browser-compat-data/pull/7246#event-3954644867 in a way that would be prevented with branch protection. I should be more careful, but also this would be great to enable :)

One non-obvious side effect of branch protection is that you end up locking in the names of the required checks, so any time in the future that you want to change your CI setup, you end up having to rebase any existing PRs to pass the new checks. I still think it's clearly worth it, though.

[ddbeck]

@foolip

One non-obvious side effect of branch protection is that you end up locking in the names of the required checks, so any time in the future that you want to change your CI setup, you end up having to rebase any existing PRs to pass the new checks.

Sorry, I don't follow this. Could you give me an example of what you mean here? If there's going to be additional hurdles for contributors, I'd like to have a good idea of what I'll need to help people with.

[foolip]

@ddbeck here's what the settings UI for requiring checks to pass looks like for WPT:

The list is populated based on the names of checks that have run in the repo. Right now, "sink-task" is one of the required checks, but it was something else when Travis was involved. When one changes which checks are required, all open PRs turn into a pending state, and typically require rebasing to trigger the new check. For WPT, this has been pretty annoying to deal with, because we have so many open PRs.

For BCD, it would be smart to think twice about what the names of the checks we require are. Make sure we're happy with them, and that they don't include anything like version numbers which will change over time.

[ddbeck]

Oh, that's really helpful. Thank you, @foolip. I've added reviewing and revising checks names as the first task at the top of the issue. 👍

[ddbeck]

8768 has been merged, so now we've now got some usefully-named build jobs.

If there are no objections, then I'm going to turn on our first branch protection rules for master on Monday, 25 January. I'm planning to select the following rules:

• _Require status checks to pass before merging_ with the following status checks selected:
  
  
  
  • _Active LTS_
  
  
  • _Maintenance LTS_
  
  
• _Include administrators_

[sideshowbarker]

Sounds good to me