Test-infra: `approve` plugin makes assumptions about OWNERS file implementation

[cjwagner]

The approve plugin is not working with regex filtering because it makes a bunch of undocumented assumptions about the implementation of the repoowners package which aren't necessarily true for regex filtered OWNERS files (thats what I get for copying code from mungegithub...)
Some invalid assumptions that are not provided by the interface:

• Approvers in OWNERS files are able to approve any file in the directory or subdirectories.
• OWNERS files contain one set of approvers.
• Files in the same directory have the same approvers.

The approve plugin will need a pretty significant rewrite to support regex filtering. It was due for one anyways; it is very inefficient and excessively complex, but I don't think I'll have the cycles to do this in the near future.

Other OWNERS file based plugins (blunderbuss, owners-label) are unaffected by this problem and should work properly with regexp filtering.

Sorry for the trouble Jeff :frowning_face:

/kind bug
/area prow
cc @ixdy @rmmh

[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

[spiffxp]

/lifecycle frozen
Also a note to self: this is the issue to link when describing why OWNERS regex filters don't work for approvers

[cblecker]

Is there clear enough direction here for this to be help wanted, @cjwagner?

[cjwagner]

Is there clear enough direction here for this to be help wanted, @cjwagner?

Definitely not. It is unclear how approver suggestions and the list of files that still need approval should work with regexp filtering.

[nikhita]

/sig testing
/sig contributor-experience

[BenTheElder]

suggestion https://github.com/kubernetes/kubernetes/pull/76162#issuecomment-480099120

[cjwagner]

That suggestion SGTM and seems clear enough.
/help

[nikhita]

/assign

I really want this functionality :)

[rlenferink]

What is the status of this? We’d like to use this for k/website as well :)

[nikhita]

I looked into the plugin and realized that it needs a lot of work (like @cjwagner mentioned), just haven't had the time to implement it yet.

[matthyx]

Since @nikhita owns it...
/remove-help

[mmitoraj]

Hi, could you tell me what are the plans for the approve plugin to work with regex filtering? Can I expect the feature in the near future? :)

[stevekuznetsov]

@mmitoraj I don't think there are any current plans to make this better, but we are always happy to provide reviews and guidance if someone wants to pick this one up!

[hudymi]

As this is something that blocks our integration with Tide, I can try to solve this issue.

To start working on it, I need a clear statement of what is the expected behavior because I only tried Tide, I don't use it on a daily basis - we are using GitHub codeowners functionality currently.

If it is hard to describe expectations I can prepare a proposition to consider, but first I need to dig into approve plugin :)

[cblecker]

@michal-hudy Thanks for your interest! Let me see if I can sketch out some design goals.

/unassign @nikhita
/assign

[cblecker]

As @cjwagner said in the original issue, approve makes a bunch of assumptions about how it reads and handles OWNERS files.

Instead of tracking things on a directory by directory basis, approve needs to track the approval status of each file. It also needs some UX changes to be able to display and represent what files may need to be approved, if only some files in a directory have been approved. The UX needs to be able to balance being verbose enough to clearly indicate what still needs approval, without being overwhelmingly long.

As an example of what a regex OWNERS file looks like, you can check out https://github.com/kubernetes/kubernetes/blob/master/OWNERS. In that file, ixdy should be able to approve any *.bzl, BUILD, or BUILD.bazel files in both the root directory, as well as any sub directory.

Let me know if this clears up what the goals if this are, and if you have any other questions :)

/unassign

[dims]

/cc

[matthyx]

@nikhita do you have time to come back to this? Do you want me to have a look?

[liggitt]

we also would use this to assign targeted reviewers/approvers for CHANGELOG*.md files in the repo root

[dudicoco]

What is the status of this issue?

[matthyx]

@dudicoco looks like nobody's currently working on it...
If you need that fix, I can take a look, or if you want to work on it that's possible too!

[nikhita]

@ykakarap and I are working on it. We have a POC that works already!

We are writing up more tests and will create a KEP soon. :smile:

[matthyx]

Awesome!

[sttts]

🎉 thanks!