alertmanager: add Slack app token support to AlertmanagerConfig CRD#8243
alertmanager: add Slack app token support to AlertmanagerConfig CRD#8243pharaujo wants to merge 12 commits intoprometheus-operator:mainfrom
Conversation
27122bb to
50d0572
Compare
simonpasquier
left a comment
There was a problem hiding this comment.
Thanks for the PR! Can you split the change into 2 PRs? The first one would be the support in pkg/alertmanager/types.go and the second one the changes to the AlertmanagerConfig CRDs. It makes it easier to review.
@simonpasquier I had opened the one already for the am config, maybe this can cover the crd changes #8238. /cc @pharaujo |
|
Sure, I'll rebase, thanks for the heads up! |
eea3cd1 to
400e11b
Compare
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
400e11b to
4e77425
Compare
|
@simonpasquier I've rebased, so now the PR is focused on the CRs. |
1d5f74b to
c934d91
Compare
Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
c934d91 to
685b7a2
Compare
Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
|
@heliapb are you able to take another look? Thanks! |
|
@simonpasquier are you able to have a look, please? |
912d251 to
082bc3c
Compare
|
@simonpasquier @heliapb am I missing anything else? Can I make any change to make reviewing easier? I feel the XL tag is misleading, as most code in the PR is generated; the core change is actually quite small. |
| // slackAppUrl defines the default URL for Slack App message posting API. | ||
| // It requires Alertmanager >= v0.30.0. | ||
| // +optional | ||
| SlackAppURL *URL `json:"slackAppUrl,omitempty"` |
There was a problem hiding this comment.
| SlackAppURL *URL `json:"slackAppUrl,omitempty"` | |
| SlackAppURL *URL `json:"slackAppURL,omitempty"` |
There was a problem hiding this comment.
I understand what you're saying, the SlackConfig is appURL so it's inconsistent. I noticed it as well, but the other URLs in GlobalConfig are slackApiUrl, opsGenieApiUrl, pagerdutyUrl, so I thought it would make sense to keep consistency within the config scope. Do you still want me to change this?
There was a problem hiding this comment.
@pharaujo Last time when I did the same (try to align with the former approach), the maintainers prefer to go with the newer approach when implementing new variables, although it is not aligned with the previous variables.
Let's wait for @simonpasquier to give a guidance. :)
Description
Slack app support released in alertmanager v0.30.0, (implemented in prometheus/alertmanager#4211).
Supporting the new fields in the Alertmanager config was done in #8238, and now this one adds support for them in the operator CRs.
Additionally added SlackConfig tests I had around.
Closes #7967
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
xin 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.)Verification
Added tests to validate functionality. Additionally, my company has been running a prometheus-operator fork with this patch since last February (original patch here: 8fd1df7)
Changelog entry