Skip to content
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

Add VictorOps Receiver type to AlertmanagerConfig CRD #3701

Merged
merged 3 commits into from Nov 24, 2020

Conversation

sev3ryn
Copy link
Contributor

@sev3ryn sev3ryn commented Nov 20, 2020

fixes #3539

@sev3ryn sev3ryn marked this pull request as ready for review November 20, 2020 10:02
@sev3ryn sev3ryn requested a review from a team as a code owner November 20, 2020 10:02
@simonpasquier
Copy link
Contributor

Thanks! cc @grdryn

@sev3ryn sev3ryn force-pushed the victoropts branch 4 times, most recently from 5df2d9f to 1ccc228 Compare November 20, 2020 18:17
@lilic
Copy link
Contributor

lilic commented Nov 24, 2020

@sev3ryn seems like this needs another rebase. :) Thanks for the hard work and patience!

@sev3ryn
Copy link
Contributor Author

sev3ryn commented Nov 24, 2020

@lilic I'll rebase this first to be merged and #3697 after this will be merged to master as it will also have conflicts

@lilic
Copy link
Contributor

lilic commented Nov 24, 2020

Sounds like a good plan!

@sev3ryn
Copy link
Contributor Author

sev3ryn commented Nov 24, 2020

@lilic rebased and also fixed example in documentation - see 6f760d3

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice :) A few remarks only.

pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
pkg/apis/monitoring/v1alpha1/alertmanager_config_types.go Outdated Show resolved Hide resolved
@@ -444,6 +446,29 @@ type EmailConfig struct {
TLSConfig *monitoringv1.SafeTLSConfig `json:"tlsConfig,omitempty"`
}

// VictorOpsConfig configures notifications via VictorOps.
// See https://prometheus.io/docs/alerting/latest/configuration/#victorops_config
type VictorOpsConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we miss CustomFields here? This should be []KeyValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see CustomFields is alertmanager configuration
https://prometheus.io/docs/alerting/latest/configuration/#victorops_config

if it is planned in future alertmanager release I believe it should be added later. But anyways if you want it to be added I'll add

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have added the link to the code in the first place :)
https://github.com/prometheus/alertmanager/blob/a7f9fdadbecbb7e692d2cd8d3334e3d6de1602e1/config/notifiers.go#L483

The documentation is out-of-date unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 348657e.

@sev3ryn
Copy link
Contributor Author

sev3ryn commented Nov 24, 2020

before merging this PR - please let me know - so I'll rebase all the fixup commits - to make git history nicer :)

@simonpasquier
Copy link
Contributor

thanks for the quick update! We can squash all the commits before merging so no real need for you to autosquash things.

@simonpasquier
Copy link
Contributor

lgtm

@simonpasquier simonpasquier merged commit 0102b96 into prometheus-operator:master Nov 24, 2020
@sev3ryn sev3ryn deleted the victoropts branch November 24, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add VictorOps Receiver type to AlertmanagerConfig CRD
5 participants