-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Discord integration #5671
Discord integration #5671
Conversation
Signed-off-by: Azanul <azanulhaque@gmail.com>
Signed-off-by: Azanul <azanulhaque@gmail.com>
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.
Thanks! I've only looked at the API additions for now.
// The secret needs to be in the same namespace as the AlertmanagerConfig | ||
// object and accessible by the Prometheus Operator. | ||
// +optional | ||
APIURL *v1.SecretKeySelector `json:"apiURL,omitempty"` |
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.
this should be a required field? IIUC there's no global Discord URL parameter and Alertmanager will fail to load a discord config with an empty URL.
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.
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.
Strange, I can't seem to find discord configs in the docs, although I can see the support by reading Alertmanager's codebase.
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.
I guess the documentation on the website only shows the global config, and indeed Discord is not listed there:
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.
It seems like it was supposed to be released but hasn't been yet.
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
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.
Just two comments, but overall looks good! Would you mind adding some unit tests to amcfg_test.go
Co-authored-by: Joao Marcal <joao.marcal12@gmail.com>
Co-authored-by: Joao Marcal <joao.marcal12@gmail.com>
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.
Adding a test and addressing Simon's comment and it should be good!
Thanks for the contribution :)
Signed-off-by: Azanul <azanulhaque@gmail.com>
8ba088b
to
7a3cd44
Compare
Signed-off-by: Azanul <azanulhaque@gmail.com>
Co-authored-by: Arthur Silva Sens <arthursens2005@gmail.com>
docs improvement Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
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.
lgtm, just have to fix the ci, part of it is just running make format
Apologies @Azanul maybe |
No file change, here's the log
|
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
There's no such field in the Alertmanager configuration. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
And add validation on v1beta1 version. Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
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.
Thanks @Azanul for the contribution and @simonpasquier for the final push :)
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Description
Prometheus Alertmanager brings the functionality of pushing notifications to Discord using webhooks.
Fixes #5251
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
x
in the box that apply.CHANGE
(fix or feature that would cause existing functionality to not work as expected)FEATURE
(non-breaking change which adds functionality)BUGFIX
(non-breaking change which fixes an issue)ENHANCEMENT
(non-breaking change which improves existing functionality)NONE
(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Changelog entry
Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.