-
Notifications
You must be signed in to change notification settings - Fork 5
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
batch - support integration-name instead of integration-ID #32
batch - support integration-name instead of integration-ID #32
Conversation
@nicolastakashi I guess charts also have to be updated? |
We need to update the CRDs and also the Image version. |
so to leave it to different PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome Or, thanks for tackling this one!
I left a few comments regarding error handling. It should be good to merge after that
Also food for thought for a future improvement, we could avoid multiple calls to the webhook API if we do a single one when the operator is initializing and cache those webhooks in memory. The operator itself can provide the webhook IDs, instead of calling the Webhook API once per Alert resource |
TBH I'm not a big fan of handling states inside the operator, but I guess we won't have a choice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome Or! Thanks for tackling this so quickly
I've added just a few more comments, but they are non-blocking. Feel free to merge or to address them, it is up to you :)
e.g. -
integrationName: WebhookAlerts
instead ofintegrationID: 2235