The default branch should require a reviewer

[plehegar]

In the spirit of avoiding unchecked changes to specifications, we should push our Groups to require reviews for Pull Requests.

[plehegar]

Note: we have around ~175 repositories where the default branch protection don't enforce a reviewer

[foolip]

Do you have a list of repos without branch protection? Perhaps asking ten of them at random would help inform what the impact of this would be?

[plehegar]

Very few specs are without protection now in WGs:
https://w3c.github.io/validate-repos/report.html?grouptype=workinggroup&filter=unprotectedbranch
We still need to chase CGs, in particular WICG.
We don't have the list of repos without reviewers., but the info has been added to
https://raw.githubusercontent.com/w3c/validate-repos/master/report.json
so, getting the list is a matter of adding 2/3 lines of code I think. Will get to it by Monday. I mentioned this to all team contacts to go after.

[foolip]

From blink-dev discussion it sounds like @marcoscaceres and @yoavweiss wanted to move in this direction for WICG. However, just like for everything else, asking the people it will affect first would be a good idea :)

[marcoscaceres]

Definitely want to start enforcing this.

[foolip]

Could this same code base be used to WICG?

[foolip]

@dontcallmedom would it be possible for me to have write access to this repo in order to experiment with access to the necessary GitHub token?

[plehegar]

@foolip sure, but I'll let the WICG Chairs step-in.

@marcoscaceres @travisleithead @yoavweiss
you can find the list of repositories with unprotected branches using
https://www.w3.org/PM/Groups/chairboard.html?gid=80485

If a repo is improperly listed, its w3c.json needs to be updated.

Note that the underlying data is (hopefully) regenerated every 24 hours, so it's NOT a live dashboard.

[travisleithead]

Per https://www.w3.org/PM/Groups/chairboard.html?gid=80485 I fixed the two repos without w3c.json files. I'm 👍 for enabling required reviewer protections for PRs.

[foolip]

Thanks all!

One thing that's missing from this repo to make it work for checking WICG branch protection is changing the condition for when branch protection is required. Right now its "any spec published from this repo is on the Rec track". What should the condition be for WICG? If it could simply be required for all repos that'd certainly be the easiest, but is it risky?

[plehegar]

to make sure I understand, you're suggesting to fix:
https://w3c.github.io/validate-repos/report.html?filter=unprotectedbranch%2Cmissingashnazghook&grouptype=communitygroup
which doesn't say anything about branch protection despite the option. Correct?

[dontcallmedom]

@foolip I think one test you could rely on is if the w3c.json data includes for repo-type cg-report (which would allow to ignore repos that used for administrative purposes, testing, etc)

[foolip]

@plehegar yes, that's right, I'd like to list missing branch protection for more things than currently require ash-nazg. The rule that @dontcallmedom suggested might work, I'll see what the code changes would look like.

[foolip]

(I sent WICG/speech-api#73 for one missing w3c.json.)

[foolip]

I've submitted a draft fix for this in #66 with some questions on the PR. @plehegar you might want to have a look at the "who should be consulted" bit.

[plehegar]

If I understood the code, it relies on the contacts info in w3c.json, which seems appropriate to me.

[plehegar]

(side note, I protected the main branches of all of the WICG repos)

[cwilso]

Also: also a WICG chair. :)

[cwilso]

(side note, I protected the main branches of all of the WICG repos)

What does that mean exactly?

[plehegar]

Oops. sorry Chris. Didn't ping you earlier.

re branch protection, see https://help.github.com/en/github/administering-a-repository/about-protected-branches .

The level above that is to enable required reviews for pull requests:
https://help.github.com/en/github/administering-a-repository/enabling-required-reviews-for-pull-requests

[foolip]

Actually no, while the presence of contacts is checked (as 'incompletew3cjson') it's just adjacent in the code, this new error isn't dependent on it.

The question is rather who would look at https://w3c.github.io/validate-repos/report.html and work with groups to enable required review.

However, given #66 (comment) I think it's premature to worry about that. First the error must exist, then we can talk with a few groups along the "no review ever" ↔ "required review" spectrum to understand how this would work in practice.

[foolip]

Here's a screenshot of the current branch protection settings in GitHub's UI:

@plehegar do you know what just enabling branch protection without enabling any of these settings does? I'm guessing it will only prevent force pushing or deleting the branch?

The different levels I had in mind for https://github.com/w3c/validate-repos (not updated yet) were:

• just branch protection, whatever that currently does
• "Include administrators" also checked, to avoid accidental pushes, which does happen at least in wpt
• "Require pull request reviews before merging" with 1 required reviewer

The last is the one that would be the most burdensome, because it would require pull requests to be created for every change, and for it to be reviewed by someone else with write access.

[foolip]

Two new checks have landed now, this is the most interesting one:
https://w3c.github.io/validate-repos/report.html?filter=norequiredreview

[foolip]

I wrote a script in https://github.com/foolip/spec-review-stats to get a sense for what review looks like in various repos. Some I tested:

• w3c/web-nfc: 109/146 approved (75%)
• w3c/webrtc-pc: 107/155 approved (69%)
• WICG/portals: 40/52 approved (77%)
• WICG/webpackage: 101/108 approved (94%)
• whatwg/fetch: 12/21 approved (57%)
• whatwg/html: 191/191 approved (100%)

I didn't come across a repo where review never happens, but this was just a sampling.

It seems like when there isn't an approving review, usually the reviewer just merged, which is implicit approval. For repos where that's the only exception to review, there presumably it would be only a minor inconvenience to require approving review.

[plehegar]

I agree that activating this for Groups that do the reviews but didn't activate the checks would be minor inconvenience and we could do this. In addition, there is value in surfacing those numbers to the Groups. Numbers and colorful graphics do help surfacing issues in general and educating the community at large. I suggest raising a separate issue to track this idea. Not sure who has the cycles to take your code to the next step however but, unless you're willing to continue iterating, I can ask around.

[foolip]

I do intend to keep tinkering here towards the goal expressed in this issue, but I'd also love to collaborate with others, if there's anyone on W3C staff or elsewhere who'd like to work on this.

[domenic]

Several of my WICG specs (e.g. WICG/origin-policy, WICG/import-maps) have been light on reviews, and I'm not sure I'd be able to find another engineer with the time and engagement to provide reviews, especially during the pre-implementation stage of incubation. That said, I'm strongly in favor of requiring this more generally, so I don't want to stand in the way of progress...

[foolip]

@domenic I've heard similar things from both @annevk and @jyasskin so I think that strongly requiring PRs with approved review is probably the wrong tradeoff. It's probably best to start light on enforcement and look for where review isn't happening. If those seem unproblematic, there would be no reason to raise the bar further.

[jyasskin]

@foolip FWIW, I skip review for some trivial changes like typo fixes or fights with TravisCI. I'd welcome a way to mark those in the CL description and then require that marking for any unreviewed commit. I'm sad when I can't find a reviewer for substantive changes, and I'd be happy to trade such reviews with @domenic.

[annevk]

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.

[marcoscaceres]

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

Maybe we can leave the ability for "admins" to push?... this can include one or two primary editors. I often make little cross-reference fixes to documents, or fix things in the CI scripts, and probably wouldn't want to bother folks to review those.

Anything else, I agree that having PR for everything is great.

[annevk]

I think even admins should create a PR and shouldn't be able to push to master. Even trivial fixes can contain typos or xref issues that CI will catch. (Note that requiring a PR and a CI run does not necessarily mean requiring review.)

[marcoscaceres]

Can't disagree with that... I know I've committed my fair share of typos, etc. by just pushing and not waiting for CI checks, etc.

[cwilso]

If this were a standards-track specification, I would agree. I *DON'T* agree that all commits to an early incubation need review/PR as a matter of course; I think the effect of such a rule would be to push the process of early iteration back into personal repos, and by the time it gets to WICG it will be considered "baked". That would be a shame.

…

On Mon, Dec 30, 2019 at 9:00 PM Marcos Cáceres ***@***.***> wrote: Can't disagree with that... I know I've committed my fair share of typos, etc. by just pushing and not waiting for CI checks, etc. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AAD3Y6I7DELGIPE63DUKQGDQ3LGYBA5CNFSM4G637H52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH3XB6Y#issuecomment-569864443>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3Y6JOBW6HGD4DTV76MJDQ3LGYBANCNFSM4G637H5Q> .