Feat: Add WeChat in Global Config #7627
Conversation
nutmos
force-pushed
the
feat/add-wechat-global-config
branch
from
0fd106d to
0f61c4e
Compare
AshwinSriram11
left a comment
AshwinSriram11
left a comment
There was a problem hiding this comment.
LGTM for the most part, just a few suggestions. Maybe @simonpasquier could also take a look
pkg/alertmanager/amcfg_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "valid global config telegram api url", |
There was a problem hiding this comment.
It might be better to add Telegram test cases in a separate PR.
There was a problem hiding this comment.
@AshwinSriram11 Both Telegram and Jira are duplicated from the existing config that was already merged. I removed all of them in the latest comment already.
pkg/alertmanager/amcfg_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "valid global config jira api url", |
pkg/alertmanager/amcfg_test.go
Outdated
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "invalid global config wechat config missing secret", |
There was a problem hiding this comment.
We can also have a test case for an empty APICorpID
There was a problem hiding this comment.
@AshwinSriram11 I tried writing some test cases but it seems not to produce error as expected. Is it expected to be tested in E2e?
There was a problem hiding this comment.
I don't think so, maybe we need to validate this manually in the convertGlobalWeChatConfig
Let's wait for @simonpasquier or @slashpai to confirm this
There was a problem hiding this comment.
Ideally we could test in the e2e tests but the validation rules are simple enough that I wouldn't mandate it. It's required for more complex validations like regex.
AshwinSriram11
left a comment
AshwinSriram11
left a comment
There was a problem hiding this comment.
Other than the one test case we discussed, everything looks great 👍
| out.WeChatAPISecret = apiSecret | ||
| } | ||
|
|
||
| if in.APICorpID != nil { |
There was a problem hiding this comment.
Like I mentioned, maybe we need to do the check here but let's wait
nutmos
force-pushed
the
feat/add-wechat-global-config
branch
from
427c2ae to
bf00bd4
Compare
pkg/alertmanager/amcfg.go
Outdated
| } | ||
|
|
||
| if in.APICorpID != nil { | ||
| if *in.APICorpID == "" { |
There was a problem hiding this comment.
it should be prevented by the kubebuilder validation.
There was a problem hiding this comment.
I'd like to do so but @AshwinSriram11 mentioned before to have a check logic implemented here as well. I'm not sure what is the practice I should follow.
There was a problem hiding this comment.
Sorry if I misguided you. I was not sure what would be the correct method and asked for @simonpasquier's advice. I would suggest proceeding with the original method
There was a problem hiding this comment.
@AshwinSriram11 No problem. I will proceed with the original way.
|
@slashpai @simonpasquier This is ready to review now. |
3 participants
Description
Add global config support in the alert manager for WeChat.
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
Using the unit test for validation.
Changelog entry
Relate #7622