Branch protection rules for all repos

[foolip]

This is a subset of #113.

In w3c/validate-repos#27 it's being discussed to require review for the default branch in W3C repos, and as part of that I've worked on that tooling. There are things currently being checked:

• is the default branch protected?
• is review required on that branch?
• is this enforced for admins as well?

I've run the tool with some tweaks to extract the state of WHATWG repos:

repo	branch protection	review required	admin enforced
books	☑	☑	☑
compat	☑	☐	☑
console	☑	☑	☑
dom	☑	☐	☐
encoding	☑	☐	☐
fetch	☑	☐	☐
figures	☑	☑	☑
fullscreen	☑	☑	☑
html	☑	☑	☑
html-build	☑	☑	☑
infra	☑	☐	☑
loader	☑	☑	☑
meta	☑	☑	☑
mimesniff	☑	☐	☐
misc-server	☑	☐	☑
notifications	☑	☐	☐
participant-data	☑	☐	☐
participate.whatwg.org	☑	☐	☑
platform.html5.org	☑	☑	☑
quirks	☑	☑	☑
sg	☑	☑	☑
storage	☑	☐	☐
streams	☑	☑	☑
url	☑	☐	☑
wattsi	☑	☑	☑
web-history	☑	☑	☑
whatwg.org	☑	☑	☑
xhr	☑	☐	☐

(Updated 2019-12-17)

I would argue we should enable branch protection and required review for all repos, since this is effectively how everyone is already operating. The admin enforcement is less important, and enabling it creates more work if one needs to bypass flaky CI, so I propose no change there.

@whatwg/editors what do you think?

[foolip]

Also @sideshowbarker for html-build and a few other repos.

[foolip]

There's also a "Require linear history" setting which we might want to enable?

[sideshowbarker]

I would argue we should enable branch protection and required review for all repos

Yes, agreed

since this is effectively how everyone is already operating.

Right (and so to be clear) when somebody doesn’t do that — and pushes to the master branch without those steps — it’s therefore most likely an accident (e.g., somebody thinking they’re on a PR branch and do git push -f or whatever to update the branch, but actually being on the master branch). And putting the protections in place will prevent those accidents.

There's also a "Require linear history" setting which we might want to enable?

Yes, I think we should do that also — again, because that’s effectively how everyone is already operating and for these repos I don’t know a reason why anybody would want to do otherwise. (I contrast, for WPT, I vaguely recall James or others giving some reasons for wanting to do otherwise sometimes — though I don’t remember the details…)

[foolip]

Aside: I think we could enable linear history for WPT, but we have in the past merged in other repos and it might happen again.

In 23cac64 I gave it a shot to check that the branch protection settings conform to some rules I expressed in JSON schema, which was surely overkill.

Anyway, I think making that script say OK for all repos should resolve this issue, although the enforce_admins bit is up for debate.

[foolip]

I've gone ahead and enabled/tweaked some rules on repos that I've contributed to a bunch myself where I'm confident it won't be controversial, like misc-server.

[GPHemsley]

I agree with enabling the three columns in the table - note that admins by virtue of their adminship can temporarily disable "admin enforced" if they really truly need to bypass these restrictions in certain cases.

[domenic]

Enabling more lockdowns makes sense to me. I'm not sure I understand the distinction between the two types of green checkmarks in the table, but I guess they will all become a single type of green checkmark, which is probably good.

[foolip]

@domenic unfortunately the usual interactive checkboxes [ ] didn't work in tables so I went looking for alternatives and found :white_check_mark: and :heavy_check_mark:. I guess I shouldn't be surprised that it renders differently in different fonts. I've replaced with ☐ and ☑.

[foolip]

@domenic for https://github.com/whatwg/participant-data it looks like @whatbot needs to be able to push without review. Do you know if this is the case for any other repos as well?

[foolip]

For https://github.com/whatwg/misc-server and https://github.com/whatwg/participate.whatwg.org there's also a bit of a wrinkle, namely that dependabot is enabled, and it won't be possible to have automatic merges or even saying @dependabot merge without first giving approving review. For now I've enabled the other things for those, but not required review.

[foolip]

I've updated the table with the current state of things, after enabling branch protection to avoid accidental deletion and force pushing, but only adding "review required" where there was some positive signal. "admin enforced" is a bit inconsistent, but all repos that don't have it also don't have "review required" so I'll come back to it.

@annevk says on chat that for some repos editorial changes are merged if nobody shows up to review, and whatwg/dom#805 is a recent such case.

I'll wait until @annevk can comment after the holidays before continuing here. If anyone thinks any setting is getting in the way of work on some repo, please just disable it and comment here. It could well be that GitHub doesn't have the flexibility we need, and we shouldn't maximally lock things down for all repos.

🎄 We'll see.

[domenic]

@domenic for https://github.com/whatwg/participant-data it looks like @whatbot needs to be able to push without review. Do you know if this is the case for any other repos as well?

Also for participant-data-private

For https://github.com/whatwg/misc-server and https://github.com/whatwg/participate.whatwg.org there's also a bit of a wrinkle, namely that dependabot is enabled, and it won't be possible to have automatic merges or even saying @dependabot merge without first giving approving review. For now I've enabled the other things for those, but not required review.

I'm OK with that; I need to manually review dependabot's work anyway, and pressing the merge button is not significantly better or worse than adding a comment.

[foolip]

pressing the merge button is not significantly better or worse than adding a comment

It's not clear from the comment if you know this, but in case not, the required steps will actually be switching to "Files changed" tab and approving there, and then either merging or saying @dependabot merge.

In other words, the implicit approval by just merging isn't possible with required review, so there is some added overhead.

[foolip]

@annevk now that we're both back, do you have an update on what settings you'd like for repos that you're the editor for?

[annevk]

See w3c/validate-repos#27 (comment):

I think requiring PRs (and in particular CI that validates a number of things) is a must. Direct pushes shouldn't work.

Then, ideally a repository has an active enough community for review, but I find that even for whatwg/dom reviews can be hard to come by, in particular for editorial changes. And for non-editorial changes it might be an implementer who does the review (which can also take a long time unfortunately) and doesn't necessarily have the review authority for a green checkbox.

On the other hand, requiring review might motivate finding improvements here.

Not sure if that's concrete enough.

The other wrinkle here is that we have many standards with a single editor. As the editor is also an admin in many cases they can give others write access allowing them to be a reviewer, but there's no good process around that which would scale beyond what we do today.

[snuggs]

@annevk in these cases perhaps a worthy breaking of said rules is to let admin use their privileges to merge?

This way admin can feel free to add someone only when extra step becomes annoying. And organization doesn't have to create special conventions for admin=reviewer situations.
Just a thought.