-
Notifications
You must be signed in to change notification settings - Fork 1
Modifications for supporting migration of notification channels #19
Conversation
pkg/api/alertmanager.go
Outdated
@@ -304,5 +304,7 @@ func (r *ApiReceiver) Type() ReceiverType { | |||
} | |||
|
|||
type GrafanaReceivers struct { | |||
// required: true | |||
DefaultReceiver *GrafanaReceiver `yaml:"default_receiver" json:"default_receiver"` |
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.
What is default receiver config on a receiver for?
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.
Now that we support routing config, I think that can take care of the default config?
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.
I have added it based on this but I may have misinterpreted.
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.
as @codesome says, not needed now that we know that routing will be supported. Can configure a receiver on the root route for same effect
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 for clarifying it; I will remove it.
pkg/api/cortex-ruler.go
Outdated
@@ -157,6 +157,8 @@ type ExtendedUpsertAlertDefinitionCommand struct { | |||
NoDataState NoDataState `json:"no_data_state" yaml:"no_data_state"` | |||
ExecutionErrorState ExecutionErrorState `json:"exec_err_state" yaml:"exec_err_state"` | |||
Settings map[string]interface{} `json:"settings" yaml:"settings"` | |||
// Receivers are optional and used for migrating notification channels of existing alerts | |||
Receivers []*GrafanaReceiver `json:"receivers" yaml:"receivers"` |
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.
I think this should be a list of strings (receiver names), not full receiver configs
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.
+1, I was expecting it to be a list of strings and the backend will do the matching
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.
Agree as well. I think these should already be defined in the config object and possibly just referenced here.
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.
I expected this :)
I will change it to a list of strings.
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.
Oops, never submitted this review earlier 😱
pkg/api/cortex-ruler.go
Outdated
@@ -157,6 +157,8 @@ type ExtendedUpsertAlertDefinitionCommand struct { | |||
NoDataState NoDataState `json:"no_data_state" yaml:"no_data_state"` | |||
ExecutionErrorState ExecutionErrorState `json:"exec_err_state" yaml:"exec_err_state"` | |||
Settings map[string]interface{} `json:"settings" yaml:"settings"` | |||
// Receivers are optional and used for migrating notification channels of existing alerts | |||
Receivers []*GrafanaReceiver `json:"receivers" yaml:"receivers"` |
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.
Agree as well. I think these should already be defined in the config object and possibly just referenced here.
default_receiver