Fix root OWNERS approvers

[dims]

Preface: IMO, we should not fix root OWNERS approvers by adding more people to it. We should fix it by distributing the 
responsibility (per area) to SIG TLs, and sub-project TLs. I actually think we should consider removing folks from root OWNERS approvers.

Follow up from: #85638 (comment)

@bgrant0607 @brendandburns @dchen1107 @lavalamp @liggitt @smarterclayton @thockin @wojtek-t , your thoughts please!

It's been a while since the previous discussion in #85638. So at this point either we move forward by implementing what @lavalamp mentions above or document steps to add someone to the root OWNERS. Let's pick one and move on.

[dims]

/sig contributor-experience

[dims]

/committee steering

[thockin]

Without a way to self-limit, I think the group should be kept very small. As I think about it, I intentionally DON'T approve PRs because I don't want it to apply too broadly.

I suggested on slack for contribex: What if I could /approve <dir> and that would only count as approval for that specific
directory? I often want to /approve an API change but not the whole PR. Talking to other root-OWNERs (overly-empowered are we) it seems like a useful self-limit on authority.

[dims]

@spiffxp @nikhita WDYT? (looked up the /approve plugin and saw your finger prints there)

[spiffxp]

I want to get @cjwagner's input, I think he's got the most prior experience with approvers and repoowners

I think it's a good idea. My gut tells me it would be somewhat involved, N weeks ish

[spiffxp]

I also think, separately, that we should consider hitting root OWNERS a failure mode and immediately address by adding OWNERS in a deeper/more relevant dir.

It used to be that the approval message would suggest the fewest possible approvers who could cover N OWNERS files but I think it now outputs leaf approvers.

Something like: either the root owner should boostrap a deeper file with themselves and then seek out more appropriate people, or the author of the PR shouldn't be allowed to merge it until they've added an OWNERS file that would rectify the situation going forward.

[bgrant0607]

Regarding removing people from root approvers, I volunteer to be moved to emeritus status, as I can't respond to most/any PR approval requests, anyway.

[bgrant0607]

Do we have a way to audit uses of root OWNERS approval power? If not, that would be useful to implement.

I used to get regular pings to approve changes to CHANGELOG.md. CONTRIBUTING.md and .gitignore were recently updated.

go.mod already has a distinct reviewer set.

[BenTheElder]

Unfortunately the go.mod reviewer set cannot actually approve (it doesn't function) so mostly it just results in auto assigning the entire team to review every PR touching go.mod, with liggitt doing the actual approval typically. I don't think such a tool exists.

…

On Mon, Aug 3, 2020 at 6:01 PM Brian Grant ***@***.***> wrote: Do we have a way to audit uses of root OWNERS approval power? If not, that would be useful to implement. I used to get regular pings to approve changes to CHANGELOG.md. CONTRIBUTING.md and .gitignore were recently updated. go.mod already has a distinct reviewer set. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#93637 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHADK6ZMT7PDWME43VFHT3R65MWVANCNFSM4PSZIRBA> .

[BenTheElder]

Something like: either the root owner should boostrap a deeper file with themselves and then seek out more appropriate people, or the author of the PR shouldn't be allowed to merge it until they've added an OWNERS file that would rectify the situation going forward.

plenty of files can't be moved out of the root easily (think go.mod, go.sum, .gitignore ...) and lots of tools are unhappy with symlinks.

[brendandburns]

I'm with @bgrant0607 that I'm ok being moved to emeritus, I don't think I have approved a PR in > 1 year. I'm also happy to continue if that is preferrable, as can be seen from the stats, I'm not abusing the power :)

[cjwagner]

This sounds good to me, here are my two cents:
Addressing kubernetes/test-infra#7690 by rewriting the approve plugin would allow the plugin to support the OWNERS file regular expression filtering. That would help with defining different approvers for files in the same directory like what might be desired for go.sum or files with specific extensions. Adding support for /approve <dir|file> also sounds like a good way to support more granular approvals, but that would similarly be difficult to add to the existing approve plugin implementation. So the approve plugin would need a significant revamp as Aaron mentioned, but it seems worthwhile and overdue to me.

Forbidding root level OWNERS approval unless the PR includes an update to the OWNERS files such that root approval would not be required once merged is an interesting idea. That would mean that root level approvers would really only be responsible for delegating approval permissions (possibly to themselves in non-root OWNERS though). This would definitely need to be an opt-in feature and I'm not sure where it would be best to implement (approve or verify-owners), but that seems like a good way to prevent root level OWNERS from acting as a catch all. We'd probably want to exclude regex filtered approvers in the root OWNERS so that we could delegate approvals for the root level files that Ben mentioned and not consider those root approvers.

[lavalamp]

Would it be enough to make root owners non-recursive? Would e.g. adding new top-level directories play nicely with that?

[nikhita]

I think we'll need a KEP for the approval plugin revamp before moving onto the implementation (I have previously looked at the plugin and have bandwidth to work on this now).

The KEP would cover:

• Support for granular approval - /approve <dir|file>
• Support for regex filtering for approvers in OWNERS - approve plugin makes assumptions about OWNERS file implementation test-infra#7690
  • Have an opt-in feature where catch-all-root-approvers (i.e. root approvers having privileges to approve * at root) would only be required while changing the root OWNERS file to update the regex, adding new files at root, etc.
• Ability to track usage of blanket root approval vs selective/granular approval - this doesn't need to be part of prow, but we should make sure that it plays nicely with tools like devstats, etc

prevent root level OWNERS from acting as a catch all.

Technically, once we have regex filtered approvers in place, we don't need to have catch-all-root-approvers at all. We can have "root" approvers defined for files like go.mod, go.sum, etc and have leaf approvers for each sub-directory. Whether we choose to have catch-all-root-approvers would then be a policy thing IMO. Edit: misread what @cjwagner wrote, added some more things into the KEP point above!

Added this to today's SIG Contribex meeting agenda so that we can reach consensus and look at starting work on this.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale

…

On Tue, Nov 3, 2020 at 4:41 PM fejta-bot ***@***.***> wrote: Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with /close. Send feedback to sig-testing, kubernetes/test-infra and/or fejta <https://github.com/fejta>. /lifecycle stale — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93637 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD24BUFSXOISLEMWLD66CATSN7QOTANCNFSM4PSZIRBA> .

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[nikhita]

/remove-lifecycle rotten

…

On Wed, Mar 3, 2021 at 5:38 PM fejta-bot ***@***.***> wrote: Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close. Send feedback to sig-contributor-experience at kubernetes/community <https://github.com/kubernetes/community>. /lifecycle rotten — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#93637 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD24BUD7LYPOX7AWPQSIMKDTBYRELANCNFSM4PSZIRBA> .

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[nikhita]

/remove-lifecycle stale
k/enhancements issue - kubernetes/enhancements#2768

cc @ykakarap

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.