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

alert prow plugin for no cla PR merges #279

Open
pacoxu opened this issue Sep 20, 2024 · 8 comments
Open

alert prow plugin for no cla PR merges #279

pacoxu opened this issue Sep 20, 2024 · 8 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@pacoxu
Copy link
Member

pacoxu commented Sep 20, 2024

https://kubernetes.slack.com/archives/C01672LSZL0/p1726321799185879

A prow plugin that can support sending alert to slack when a PR with cncf-cla: no label was merged.

@petr-muller petr-muller added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 20, 2024
@petr-muller
Copy link
Contributor

In this a scope-limited form, this should not be hard to write. Get a GH webhook call about a merge, inspect the PR, send a notification (we already have Slack client code in Prow).

Notification systems love to grow in scope, though 🤔 Some people will almost certainly want notifications about other events. Some people will almost certainly want notifications to different destinations than Slack. Would we need per-org / per-repo configuration surface? Like disabling the notifications for certain repos, or routing the notifications to different slacks based on repos?

Given the current state of Prow maintenance, I think we should prefer building a very simple, single-purpose thing rather than trying to come up with a new generic notification system.

One alternative could be something that simply publishes interesting metrics through Prometheus and then we could set up standard alertmanager-backed alerts for events like this.

@pacoxu
Copy link
Member Author

pacoxu commented Sep 23, 2024

One alternative could be something that simply publishes interesting metrics through Prometheus and then we could set up standard alertmanager-backed alerts for events like this.

This seems to be a better solution to me, as the cncf-cla: no is not a general case and only occurs several time a year.

  • Adding a github action like thing for all PRs seems to be too heavy
  • An alert may be a lightweight solution.

If we want to go this direction, is there an example to add an alert policy?

@pacoxu
Copy link
Member Author

pacoxu commented Sep 23, 2024

@dims @BenTheElder which solution do you prefer?

@dims
Copy link
Member

dims commented Sep 23, 2024

@pacoxu see example in https://kubernetes.slack.com/archives/CJH2GBF7Y/p1724136124406689

image

An alert like that looks like a great thing to have

@BenTheElder
Copy link
Member

We shouldn't rely on the label, the status is the source of truth.

Statuses can change after PRs merge, so the alert is only a best effort warning in any case.

If we want to be 100% sure, the CNCF CLA tooling could run an audit mode and scan all commits in the repo against their CLA database (which would surely be cheaper than us attempting to scan the github API for commit statuses).

For a weak check:

Given the current state of Prow maintenance, I think we should prefer building a very simple, single-purpose thing rather than trying to come up with a new generic notification system.

Yes, I think we could reuse some tooling from the slack alerts but I think it should be a single purpose plugin if we implement this, to keep things as simple as possible.

I think the problem with using a metric is then we still need to track down where the bad merge happened, and now we have large cardinality on the metric if we're recording it there. Versus if we just push out a slack alert (and/or completely offload this to CNCF auditing).

@BenTheElder
Copy link
Member

(Poking some CNCF folks about the subject ...)

@pacoxu

This comment was marked as off-topic.

@BenTheElder
Copy link
Member

I disagree strongly with the linked issue and I think that's an unrelated topic for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants