Test-infra: required_reviewers on PRs

[cblecker]

Proposal:
blunderbuss currently selects a certain number of reviewers from the reviewer pool. By design it doesn't select the same reviewers for every PR. However, there may be use cases (high sensitivity parts of the code base, such as API changes) where a person or github team should be cc'd on every PR that touches that part of the code.

I'd like to suggest that the OWNERS file be extended with a field for required_reviewers. Github users/teams that are listed in that section should always be added to the review list, in addition to the normal rotating reviewers (not instead of).

/area prow

[cblecker]

cc @BenTheElder @cjwagner @matthyx @fejta

[BenTheElder]

Interesting. I've had one previous request for a more advanced version of this to detect changes to flags and CC a particular reviewer.

This sounds useful and pretty straightforward to implement. Should it also support regex &/or globs so we can require reviews for specific files?

[BenTheElder]

/sig contributor-experience
/kind feature

[cblecker]

Yes, I was thinking so. I believe blunderbuss supports this already, but I think the approvers plugin doesn't.

[cjwagner]

The no_parent_owners field. May be of interest. It doesn't exactly match the behavior or what you are suggesting, but it does facilitate 'locking down' a sub directory.

[fejta]

This duplicate the behavior of CODE_OWNERS, does that matter?

[cblecker]

@cjwagner Yeah that's not quite what I'm looking for. I don't want to short circuit either the reviewers or approvers that would normally be assigned -- this would be additive.

@fejta It is similar to the CODEOWNERS behaviour with two key differences:

• CODEOWNERS must have write access
• CODEOWNERS aren't additive. It runs through the file top to bottom, and the last line that matches the file is chosen.

[fejta]

CODEOWNERS must have write access

👎 🙄 oh github

[BenTheElder]

Wow, did not know about that write access requirement, that is a major point to mention next time someone asks about the difference...

[matthyx]

Do we have a place to document these features? I saw @cjwagner speaking about doc improvement efforts during last SIG.

[cjwagner]

We have some documentation about OWNERS files here: https://github.com/kubernetes/test-infra/tree/master/prow/plugins/approve/approvers
I think that OWNERs file docs (not approve plugin specific) should live here though: https://github.com/kubernetes/test-infra/tree/master/prow/repoowners

[cblecker]

I think we have consensus to move forward on this one. Marking as available for someone to pick up.

/help

[matthyx]

Ok, then :-D
/remove-help /assign

[matthyx]

/assign

[matthyx]

/remove-help

[cblecker]

You can do both commands in a single comment, they just need to be on separate lines

[spiffxp]

@matthyx

Do we have a place to document these features? I saw @cjwagner speaking about doc improvement efforts during last SIG

@cjwagner

We have some documentation about OWNERS files here: https://github.com/kubernetes/test-infra/tree/master/prow/plugins/approve/approvers
I think that OWNERs file docs (not approve plugin specific) should live here though: https://github.com/kubernetes/test-infra/tree/master/prow/repoowners [ed: nothing here at the moment]

There is also https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md

I would rather we have just one doc. I like the one I'm linking, but I'm biased since I wrote it (based on the approvers doc, I honestly just forgot to nuke that). I recognize it mixes up the owners spec + how kubernetes uses owners + tools that use owners + (aspirational) owners file hygiene. And it's a doc, so predictably it's fallen out of date (regex filters aren't mentioned at all). Separate issue? I'm open to suggestion.

[matthyx]

/close
@spiffxp will document in https://github.com/kubernetes/test-infra/issues/5333