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.

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

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

Open
krzyzacy opened this issue Nov 7, 2024 · 5 comments
Open

Comments

@krzyzacy
Copy link
Contributor

krzyzacy commented Nov 7, 2024

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

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

@krzyzacy
Copy link
Contributor Author

krzyzacy commented Nov 7, 2024

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

BenTheElder commented Nov 7, 2024

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
Copy link
Contributor Author

krzyzacy commented Nov 7, 2024

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants