python / peps

Python Enhancement Proposals
https://peps.python.org
4.35k stars 1.5k forks source link

Add a CI check to keep `CODEOWNERS` functioning #2159

Closed brettcannon closed 2 years ago

brettcannon commented 2 years ago

I found Jelle's username misspelled in CODEOWNERS, and any time that file has a mistake in it, it stops working. I personally find the file handy, so I do want to keep it functioning, but not by occasionally asking why a new PR has no automated assignment to the PEP author/delegate.

JelleZijlstra commented 2 years ago

Maybe we can use https://github.com/mszostok/codeowners-validator#checks.

brettcannon commented 2 years ago

Maybe, although giving out an access token to the action feels a little risky. Other than PEP editors we could use https://github.com/python/devguide/blob/master/developers.csv for the list of valid GitHub usernames.

pradyunsg commented 2 years ago

Well... if you want to have careful control over the code that has access to the tokens, there are two broad options:

[^1]: GitHub does not allow pushing branches/tags that look like a commit hash, so this can't be worked around. And, it's also the main mechansim recommended for security hardening with third-party actions.

CAM-Gerlach commented 2 years ago

You could also use something like gitown as a pre-commit hook. That way, no need to give it a token, it runs as part of the existing pre-commit action step, and can also optionally be run locally, either automatically on commit or on-demand with pre-commit run.

It is more designed to automatically maintain the CODEOWNERS file by finding if an author of a customizable percent of the file isn't added as a code owner, which could potentially be useful if set high enough as an extra check that PR authors added themselves as codeowners, but on the other hand strict validation is not its primary function and it would need manual configuration (since it doesn't have token access).

So maybe not the best option, but figured it was worth mentioning since it uses the existing infra.

CAM-Gerlach commented 2 years ago

Seems like GitHub now validates CODEOWNERS. On a recent PR, e.g. #2320 , I see the following at the top of the CODEOWNERS file in the Files pane. Not sure if it blares a warning if it isn't valid, but so long as reviewers glance at the Files pane, it should be visible.

image

AA-Turner commented 2 years ago

@CAM-Gerlach out of interest if one intentionally invalidates the file, how noisy is the warning / error, if extant?

A

CAM-Gerlach commented 2 years ago

I was curious about the same thing; I was going to test but it looks like @JelleZijlstra beat me to it with #2328

JelleZijlstra commented 2 years ago

Unfortunately it turns out the check doesn't actually warn about invalid usernames in CODEOWNERS.

AA-Turner commented 2 years ago

Negative: GH doesn't validate invalid usernames

Positve: GH does validate actual syntax errors, with line level highlighting

Even more positive: actual syntax errors, invalid usernames, etc, seem to now be localised to a per-line level, and the rest of the file still works. The documentation hasn't been updated yet as far as I can tell, so this may be a soft launch, but I think this issue can now be closed as there doesn't seem to be risk of all of CODEOWNERS breaking anymore.

A

JelleZijlstra commented 2 years ago

Agree with @AA-Turner. For future reference, we tried out some things in https://github.com/JelleZijlstra/ownerstest.

CAM-Gerlach commented 2 years ago

Thanks @JelleZijlstra and @AA-Turner ! I guess I'll close this now, since we're all in agreement that this is more or less resolved.

hugovk commented 2 years ago

Even more positive: actual syntax errors, invalid usernames, etc, seem to now be localised to a per-line level, and the rest of the file still works. The documentation hasn't been updated yet as far as I can tell, so this may be a soft launch, but I think this issue can now be closed as there doesn't seem to be risk of all of CODEOWNERS breaking anymore.

The changes have now been announced:

https://github.blog/changelog/2022-02-17-codeowners-improvements-syntax-errors-preview-of-who-will-be-requested-and-more/

CAM-Gerlach commented 2 years ago

For reference, invalid usernames are now detected and enumerated, so we can consider this issue fully closed.

image