kubernetes-sigs
`require_self_approval: true` should prevent suggesting PR author for approval
[This is re-filed from https://github.com/kubernetes/test-infra/issues/29827]
What happened:
TLDR in a repo with implicit_self_approval off, prow will sometimes suggest the author as the approver, it should suggest someone else.
https://kubernetes.slack.com/archives/CDECRSC5U/p1669738130214399
What you expected to happen:
suggest non-pr-author for finding an approver
How to reproduce it (as minimally and precisely as possible):
.... you'll need implicit_self_approval disabled, yourself in OWNERS, and then file a PR until this happens.
Please provide links to example occurrences, if any:
lots of sample links in the slack thread linked above
Anything else we need to know?:
/sig testing
The Kubernetes project currently lacks enough contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue as fresh with
/remove-lifecycle stale - Close this issue with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.
This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue as fresh with
/remove-lifecycle rotten - Close this issue with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten
Approver logic is tricky but filtering out author name from the list of options does not seem too scary, so let's try marking it as a good first issue. I hope I'm not throwing someone under the bus :grimacing:
Hey @BenTheElder @petr-muller can I look into this issue? also if yes then can you suggest me some steps to get started on this?
@Bharadwajshivam28 Sure, feel free to give it a shot!
Here are some breadcrumbs. The problem is that Prow sometimes asks ...please ask for approval from AUTHOR... even when require_self_approval is set to true, indicating that authors with approval permissions do not get an automated approved label, and it is likely desirable for other approver to approve the PR (although the author can still approve it themselves, but if that behavior is desirable then the repo should disable require_self_approval.
The message seems to be built here from a Approvers struct (partially from its AssignedCCs member, partially from SuggestedCCs), you can follow its call chain to figure out how the Approver struct that ends us passed here is built and prevent the author from being added here if require_self_approval: true (unless they are the only approver, I guess).
https://github.com/kubernetes-sigs/prow/blob/24e765300acd79b8c9095558cbd00c344d033aee/pkg/plugins/approve/approvers/owners.go#L685-L691
I retitled this issue to mention the require_self_approval: true config instead, which is the current name of that config option. The original implicit_self_approve was renamed with inverted defaults to current require_self_approval in 2019.
hi, @BenTheElder, @Bharadwajshivam28, @petr-muller. Can I work on this issue? /assign @Be3751
The problem is that Prow sometimes asks ...please ask for approval from AUTHOR... even when require_self_approval is set to true, indicating that authors with approval permissions do not get an automated approved label, and it is likely desirable for other approver to approve the PR
@petr-muller It makes sense to me that prow will ask the approval from the author when there is flag enabled explicitly saying require_self_approval. Is it not contradictory that require_self_approval is set to true and then at the same time Prow in the comments does not ask the author for approval?
In other words -> doesn't require_self_approval=true actually mean that author does not get implicit approval and needs to ATLEAST explicitly approve it themselves? That is to say, other people's approval are secondary (even optional maybe) but the author's approval is required. In such case, I feel it is counter intuitive to suggest only other people for approval and not the author.
What could be better messaging in the comments -> please ask approval from LIST_OF_APPROVERS_EXCLUDING_AUTHOR and since require_self_approval is true, AUTHOR must also approve it.
In other words -> doesn't require_self_approval=true actually mean that author does not get implicit approval and needs to ATLEAST explicitly approve it themselves?
Nope, require_self_approval=true does not mean the author always must approve it, although I see how the name may indicate this. Any approver is enough (including the author, but the author needs to approve explicitly if they want to use their privilege). I think the name was selected because it is an inverted form of the original implicit_self_approve config.
@petr-muller I was working on this but forgot to assign, can you please assign this to me so that there is no confusion.
