Test-infra: Prow suggests approver from not necessary parent OWNERS

Created on 24 Apr 2019 · 12Comments · Source: kubernetes/test-infra

What happened:
Hi, I have a PR to make changes for the files under kubernetes/pkg/scheduler. From my reviewer's view, it should be enough after approved by the people belong to pkg/scheduler/OWNERS.

However, the k8s-ci-robot suggests the approvers from pkg/OWNERS before merging the PR.

I am not sure whether it is designed to do so or it is a bug. Thanks.

What you expected to happen:
Approve from pkg/scheduler/OWNERS is enough to merge the PR.

How to reproduce it (as minimally and precisely as possible):
Please check the PR history:

https://github.com/kubernetes/kubernetes/pull/76301

Please provide links to example occurrences, if any:

Anything else we need to know?:

kinbug

Source

xiuqiaoli

All 12 comments

/cc @spiffxp @bsalamat

xiuqiaoli on 24 Apr 2019

Given all the force pushes it's hard to determine what files were present in the initial PR...

fejta on 24 Apr 2019

Looks like the current files are:

pkg/scheduler/factory/BUILD
pkg/scheduler/factory/factory.go
pkg/scheduler/factory/factory_test.go
pkg/scheduler/internal/queue/pod_backoff.go
pkg/scheduler/internal/queue/pod_backoff_test.go
pkg/scheduler/internal/queue/scheduling_queue.go
pkg/scheduler/internal/queue/scheduling_queue_test.go

Which I agree should pick up pkg scheduler

fejta on 24 Apr 2019

Thanks for your quick response, @fejta. What should I do next to correct the situation? Should I tell someone under pkg/OWNERS about this and ask them to approve or the robot's decisions can be modified?

xiuqiaoli on 24 Apr 2019

@fejta It seems irrelevant with "forced" push. And actually, almost all recent scheduler PRs don't respect OWNERS.

https://github.com/kubernetes/kubernetes/pull/76973 is another data point.

Huang-Wei on 24 Apr 2019

Not sure if it's related (I don't see how it could be)...but there was an indentation error in OWNER_ALIASES introduced in https://github.com/kubernetes/kubernetes/pull/76736 (merged 6 days ago) that was fixed in https://github.com/kubernetes/kubernetes/pull/76947 (merged 7 hours ago).

Looks like https://github.com/kubernetes/kubernetes/pull/76883 is a recently approved PR (1 hour ago) touching only pkg/scheduler/ and it is considered approved by OWNERS in pkg/scheduler/OWNERS...

@Huang-Wei could you try rebasing the PR? Maybe that could retrigger the approve plugin again?

nikhita on 24 Apr 2019

🎉1

@nikhita Thanks for the info (although I'm still confused why kubernetes/kubernetes#76947 helps).

After rebasing with latest code, and do a force-push, it works now!! See https://github.com/kubernetes/kubernetes/pull/76973.

Huang-Wei on 24 Apr 2019

👍1

Oh I see why it works... Actually, I did notice the yaml indent error yesterday. And doing a pull from master eliminates that indent error, so I "thought" it's just my local master too fall behind.

(So it turns out prow will check the (OWNERS) files in your )

Huang-Wei on 24 Apr 2019

(So it turns out prow will check the (OWNERS) files in your )

Ugh yeah, this is true...it checkouts the code in your PR in the plugin.

nikhita on 24 Apr 2019

Looks like the main issue is solved. Closing.

Please reopen if needed.

/close

nikhita on 24 Apr 2019

@nikhita: Closing this issue.

In response to this:

Looks like the main issue is solved. Closing.

Please reopen if needed.

/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.