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

[Prow Plugin] Make removing sig tags operations sticking after rebases #319

Open krzyzacy opened 2 weeks ago

krzyzacy commented 2 weeks ago

Pattern we see during sig api-machinery triaging:

Twice a week, we scrape all PRs/Issues with sigs/api-machinery labeled, we manually remove the sig label for PRs falls outside of the sig. Once the PR is rebased, the sig label will be added back (respecting the OWNERS file)

We probably can provide options to make sig label removal operations sticking to reduce the triaging toils

cc @henrywu573 @jpbetz

BenTheElder commented 2 weeks ago

Why not update the OWNERS file to stop inaccurately labeling it? :-)

krzyzacy commented 2 weeks ago

The labeling itself is accurate, imagine upon rebase the touched content for that sig remains the same, but we always re-apply the label

BenTheElder commented 2 weeks ago

The labeling itself is accurate, imagine upon rebase the touched content for that sig remains the same, but we always re-apply the label

We apply the label only if your PR diff contains a file under one of the OWNERS files. It's looking at the PR's patch, not the git push. (If it's not, that's the bug, but I'm pretty sure that's not it).

When you rebase, the label gets applied again because you are in fact PRing a file that is under one of the OWNERS configured with the SIG label.

The only broken case by lack of feature currently is if someone pushes a bad diff that touches lots of files, then decides to stop modifying those files, we don't remove the label on the corrected push because we don't distinguish between how the label was added.

But in that case, pushing the PR again should not re-label it, the old label just persists once, and this case isn't that common. Checking for this on every push could eat a lot of API calls since we'd have to look for if the bot previously added the label. (Also, the labels supported are arbitrary, it's not actually just SIG labels).

On balance, trying to handle that case versus just manually fixing it ... probably isn't worth it, unless we can come up with an approach that isn't API-call expensive (AFAICT we'd have to substantially change prow to create some out of band persistent data for this).

However in the case that the PR does touch a file under one of the api machinery OWNERS, then it will be re-labeled on every push and attempting to manually remove it is the wrong fix versus updating the OWNERS files. I observe mostly the latter.

krzyzacy commented 2 weeks ago

then sounds like the triaging process is not optimal (as we currently remove the sig label for PRs we think other sig should review) - instead we should have some new mechanism, or have a new triaging state (like /sig-api-machinery-triaged, and we can update the filter to exclude these PRs)

BenTheElder commented 2 weeks ago

The label can be manually added by the author or reviewers but ... yeah I agree the process is not optimal.