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
crd: add support for source pagerduty_config option in AlertMananger CRD #6427
base: main
Are you sure you want to change the base?
Conversation
The AlertManager CRD was expected to have 1:1 fields mapped from https://prometheus.io/docs/alerting/latest/configuration/#pagerduty_config . Currently source was missing so it is added. Fixes prometheus-operator#6387
Can you add unit test as well? |
@slashpai , Added the unit test for checking source gets added in case of supported versions. go test -run ^TestSanitizePagerDutyConfig$ ./pkg/alertmanager
go test ./pkg/alertmanager Thanks a lot. |
Co-authored-by: Jayapriya Pai <slashpai9@gmail.com>
You would need regenerate and update the code as well as type is changed to pointer |
Yeah I am making the changes locally. But looking at it in details I see @simonpasquier chose it to be a string , I think Source is not an optional field but has a default value set (confusing me to believe it is optional). Here is a screenshot from https://prometheus.io/docs/alerting/latest/configuration/#pagerduty_config I think I should rather change it from optional to not being optional. Might need help. Thank you for all the patience you've had with me. |
no |
Updated the changes to make source be the string pointer type since it is an optional field. Tested it out with E2E tests on local and unit tests. |
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
Thanks for working on this
@slashpai I have not removed the Thanks a lot for all the patience this has been one of the best experiences of contributing to any Open Source project. |
I don't see why the tests are failing, as they working previously and I haven't changed anything regarding the tests that failed. @mviswanathsai, do you think you can help me with this one ? @slashpai Also I see that these checks are not in the required category anyway I can run them again to check if this was an incidental failure ? |
The test that is failing is often flaky. Rerunning the checks should sort this out. |
Will be making changes as soon as possible. Working on them |
Hi @slashpai and @simonpasquier . Would you guys like me to make any further changes on this one ? |
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! We need also changes here
prometheus-operator/pkg/apis/monitoring/v1beta1/conversion_from.go
Lines 241 to 259 in 5e6aa47
func convertPagerDutyConfigFrom(in v1alpha1.PagerDutyConfig) PagerDutyConfig { | |
return PagerDutyConfig{ | |
SendResolved: in.SendResolved, | |
RoutingKey: convertSecretKeySelectorFrom(in.RoutingKey), | |
ServiceKey: convertSecretKeySelectorFrom(in.ServiceKey), | |
URL: in.URL, | |
Client: in.Client, | |
ClientURL: in.ClientURL, | |
Description: in.Description, | |
Severity: in.Severity, | |
Class: in.Class, | |
Group: in.Group, | |
Component: in.Component, | |
Details: convertKeyValuesFrom(in.Details), | |
PagerDutyImageConfigs: convertPagerDutyImageConfigsFrom(in.PagerDutyImageConfigs), | |
PagerDutyLinkConfigs: convertPagerDutyLinkConfigsFrom(in.PagerDutyLinkConfigs), | |
HTTPConfig: convertHTTPConfigFrom(in.HTTPConfig), | |
} | |
} |
prometheus-operator/pkg/apis/monitoring/v1beta1/conversion_to.go
Lines 237 to 255 in 5e6aa47
func convertPagerDutyConfigTo(in PagerDutyConfig) v1alpha1.PagerDutyConfig { | |
return v1alpha1.PagerDutyConfig{ | |
SendResolved: in.SendResolved, | |
RoutingKey: convertSecretKeySelectorTo(in.RoutingKey), | |
ServiceKey: convertSecretKeySelectorTo(in.ServiceKey), | |
URL: in.URL, | |
Client: in.Client, | |
ClientURL: in.ClientURL, | |
Description: in.Description, | |
Severity: in.Severity, | |
Class: in.Class, | |
Group: in.Group, | |
Component: in.Component, | |
Details: convertKeyValuesTo(in.Details), | |
PagerDutyImageConfigs: convertPagerDutyImageConfigsTo(in.PagerDutyImageConfigs), | |
PagerDutyLinkConfigs: convertPagerDutyLinkConfigsTo(in.PagerDutyLinkConfigs), | |
HTTPConfig: convertHTTPConfigTo(in.HTTPConfig), | |
} | |
} |
pagerduty source
The AlertManager CRD was expected to have 1:1 fields mapped from https://prometheus.io/docs/alerting/latest/configuration/#pagerduty_config . Currently source was missing so it is added.
Closes #6387
Description
Added Source in v1alpha1 and v1beta1 altermanager_config_types and added the necessary code in amcfg.go
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.)Verification
I ran package specific tests for alertmanager package for the pagerduty config method. Here is the command :
go test -run ^TestSanitizePagerDutyConfig$ ./pkg/alertmanager
Changelog entry
Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.