Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

approve plugin makes assumptions about OWNERS file implementation #7690

Closed
cjwagner opened this issue Apr 13, 2018 · 31 comments · Fixed by ti-community-infra/test-infra#32 or #29970
Closed

approve plugin makes assumptions about OWNERS file implementation #7690

cjwagner opened this issue Apr 13, 2018 · 31 comments · Fixed by ti-community-infra/test-infra#32 or #29970
Labels
area/prow Issues or PRs related to prow kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/testing Categorizes an issue or PR as relevant to SIG Testing.

Comments

@cjwagner
Copy link
Member

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 ☹️

/kind bug
/area prow
cc @ixdy @rmmh
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/prow Issues or PRs related to prow labels Apr 13, 2018
This was referenced Apr 13, 2018
Merged
Merged
@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2018
@nikhita
Copy link
Member

nikhita commented Jul 13, 2018

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 13, 2018
@spiffxp spiffxp mentioned this issue Jul 24, 2018
Merged
@spiffxp
Copy link
Member

spiffxp commented Aug 10, 2018

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 10, 2018
@cblecker
Copy link
Member

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

@cjwagner
Copy link
Member Author

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.

@cjwagner cjwagner mentioned this issue Aug 29, 2018
Merged
@wking wking mentioned this issue Aug 29, 2018
Merged
wking added a commit to wking/openshift-release that referenced this issue Aug 30, 2018
@wking
Revert "Add OWNERS for templates"
b49db8b 
This reverts commit a3e9a3f (openshift#1279).

Using filters for approvers and reviewers is the long-term goal, but
the approve plugin can't handle this yet [1,2].

[1]: kubernetes/test-infra#7690
[2]: https://github.com/kubernetes/community/blame/19dc4bed5b05c23a69e631f96573f8ed2e0774cb/contributors/guide/owners.md#L86-L87
@wking wking mentioned this issue Aug 30, 2018
Merged
@nikhita
Copy link
Member

nikhita commented Jan 29, 2019

/sig testing
/sig contributor-experience

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Jan 29, 2019
This was referenced Apr 4, 2019
Merged
Closed
@BenTheElder
Copy link
Member

suggestion kubernetes/kubernetes#76162 (comment)

@cjwagner
Copy link
Member Author

cjwagner commented Apr 4, 2019

That suggestion SGTM and seems clear enough.
/help

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Apr 4, 2019
@nikhita
Copy link
Member

nikhita commented Apr 5, 2019

/assign

I really want this functionality :)

@rlenferink
Copy link
Member

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

@nikhita
Copy link
Member

nikhita commented May 3, 2019

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.

@nikhita nikhita mentioned this issue Dec 9, 2019
Closed
@slintes slintes mentioned this issue Dec 11, 2019
Merged
9 tasks
@evankanderson evankanderson mentioned this issue Jan 30, 2020
Merged
@cjwagner cjwagner mentioned this issue Aug 4, 2020
Closed
@dudicoco
Copy link
Contributor

What is the status of this issue?

@matthyx
Copy link
Contributor

matthyx commented Nov 24, 2020

@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
Copy link
Member

nikhita commented Nov 24, 2020

@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. 😄

@matthyx
Copy link
Contributor

matthyx commented Nov 24, 2020

Awesome!

@sttts
Copy link
Contributor

sttts commented Nov 24, 2020

🎉 thanks!

@sbueringer
Copy link
Member

@nikhita Just wanted to ask if there is an update to the status of this issue.
We would also be really interested in this feature.

@ykakarap
Copy link
Contributor

ykakarap commented Mar 5, 2021

Hi @sbueringer ,
I and @nikhita are working on this. We are introducing changes to the aprpoval plugin that addresses these issues and more.
We are working on a KEP and it will be out soon.

@liggitt liggitt mentioned this issue May 3, 2021
Merged
@sbueringer sbueringer mentioned this issue Oct 20, 2021
Closed
@Ressetkk Ressetkk mentioned this issue Dec 2, 2021
Closed
@rhuss rhuss mentioned this issue Feb 4, 2022
Merged
@evankanderson
Copy link
Contributor

@nikhita and @ykakarap -- are you still working on this, or does this need another search for an owner? (ha!)

@dprotaso dprotaso mentioned this issue Apr 7, 2022
Merged
@imiller0 imiller0 mentioned this issue May 24, 2023
Open
@tenzen-y tenzen-y mentioned this issue Jun 15, 2023
Merged
This was referenced Jun 29, 2023
Merged
Merged
@wuhuizuo
Copy link
Contributor

I implemented a fixing in #29970.

PTAL 😄

wuhuizuo added a commit to ti-community-infra/test-infra that referenced this issue Jun 29, 2023
@wuhuizuo
fix(prow/plugins/approve): support get approvers when used full config (
a95a424 
#32)

## What

Fixed: kubernetes#7690

Ref:
- PingCAP-QE/ee-ops#621

## How

- Make it be fixed without massive refactoring, and not change the
behavior and approve notifies.

---------

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@wuhuizuo wuhuizuo mentioned this issue Jul 5, 2023
Closed
wuhuizuo added a commit to ti-community-infra/test-infra that referenced this issue Sep 11, 2023
@wuhuizuo
fix(prow/plugins/approve): support get approvers when used full config (
a46744a 
#32)

Fixed: kubernetes#7690

Ref:
- PingCAP-QE/ee-ops#621

- Make it be fixed without massive refactoring, and not change the
behavior and approve notifies.

---------

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
jeremysprofile added a commit to jeremysprofile/community that referenced this issue May 20, 2024
@jeremysprofile
docs: remove outdated warning
0fc7f16 
Warning message about filters on OWNERS files not working for the approve
plugin links to <kubernetes/test-infra#7690>, which
appears to be resolved.
@jeremysprofile jeremysprofile mentioned this issue May 20, 2024
Merged
@pohly
Copy link
Contributor

pohly commented May 21, 2024

Can someone confirm that #29970 is available in production?

@BenTheElder
Copy link
Member

Something merged that long ago is definitely in production unless it was reverted.

7051401 put v20240517-ea10bd814 into prod (note this will be a commit in the kubernetes-sigs/prow repo now) and we can see at prow.k8s.io in the sidebar menu at the bottom that v20240517-ea10bd814 is being served by the frontend so we can be reasonably confident that this commit was deployed for the other components (if we wanted to look further there's a job to do the deploy and we could check the logs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/prow Issues or PRs related to prow kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/testing Categorizes an issue or PR as relevant to SIG Testing.
Projects
None yet
Milestone
No milestone