-
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
feat: adding alertmanager msteams support #6002
feat: adding alertmanager msteams support #6002
Conversation
8db39f8
to
0a80b05
Compare
0a80b05
to
02b2315
Compare
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 a nit, lgtm otherwise :)
cfc0be3
to
0da8904
Compare
We ran into the same issue today and would like to use the msteams integration. |
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 looks good in general! And sorry for the delay.
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.
same remarks than for v1alpha1
5f58141
to
d2ddab0
Compare
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.
One small nit besides the formatting issues
thanks for continuing this!
Signed-off-by: Nicolas Takashi <nicolas.tcs@hotmail.com> Co-Authored-By: ksdpmx
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
c82d5ae
to
b33a736
Compare
b33a736
to
2952911
Compare
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
2952911
to
92dd983
Compare
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
I think I just found some further mistakes which I did not notice during the review... https://github.com/prometheus-operator/prometheus-operator/blob/a10b739fb3f7a56165ba8d4e1bfb57168ddb78cd/pkg/alertmanager/amcfg.go#L642C1-L652C3
The receiver is defined as msteamsConfigs. Not msTeamsConfigs. |
When encoding go structs to yaml/json, Go uses the structs tags to write down yaml strings The tag is correct :)
This is also what the golden file is showing us Looking at alertmanager configuration docs, this is also what it is expected |
Nice job guys! Do you know when we can expect a new prometheus-operator tag to use it? |
@kj187 soon, I'm working on it :) |
* feat: adding alertmanager msteams support Signed-off-by: Nicolas Takashi <nicolas.tcs@hotmail.com> Co-Authored-By: ksdpmx
Description
Adding support for Microsoft Teams receiver, since the pr prometheus/alertmanager#3324 from the upstream project Alertmanager has been merged already. while there's no release for over half one year. and fix #5766.
Co-Authored-By: @ksdpmx
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.