search

kubernetes-sigs / prow

Prow is a Kubernetes based CI/CD system developed to serve the Kubernetes community. This repository contains Prow source code and Hugo sources for Prow documentation site.
https://docs.prow.k8s.io
Apache License 2.0
129 stars 99 forks source link

`require_self_approval: true` should prevent suggesting PR author for approval #175

Open BenTheElder opened 5 months ago

BenTheElder commented 5 months ago

[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

k8s-triage-robot commented 2 months ago

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

This bot triages un-triaged issues according to the following rules:

You can:

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

/lifecycle stale

k8s-triage-robot commented 1 month ago

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:

You can:

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

/lifecycle rotten

petr-muller commented 1 month ago

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:

Bharadwajshivam28 commented 1 month ago

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?

petr-muller commented 1 month ago

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