Support for responders in OpsGenie #131
Conversation
yuri-tceretian
force-pushed
the
yuri-tceretian/opsgenie-responders
branch
from
ec3a30f
to
72e1494
Compare
because it is already expanded.
yuri-tceretian
force-pushed
the
yuri-tceretian/opsgenie-responders
branch
from
16464e0
to
b38c9f8
Compare
| if err != nil { | ||
| return Config{}, fmt.Errorf("responder at index [%d] type is not a valid template: %v", idx, err) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
This else causes values containing templates to bypass validation. If the value of this field is for example {{ printf "lol" }} the end result will be invalid but no error will be returned.
There was a problem hiding this comment.
Agree with this. The main reason to support it is the upstream. So, I think that if a user goes this way and decides to use the template the validity of the result is in their hands.
There was a problem hiding this comment.
But why not remove the else and let the template be checked as any other value after it's evaluated?
There was a problem hiding this comment.
because the result of the template expansion could be different when it is applied with actual alerts as the context. For example {{ .CommonAnnotations.ResponderType }}
There was a problem hiding this comment.
| @@ -74,6 +86,33 @@ func NewConfig(jsonData json.RawMessage, decryptFn receivers.DecryptFunc) (Confi | |||
| raw.OverridePriority = &overridePriority | |||
| } | |||
|
|
|||
| for idx, r := range raw.Responders { | |||
| if r.ID == "" && r.Username == "" && r.Name == "" { | |||
There was a problem hiding this comment.
Reading the section for the visibleTo field and the examples for both responders and visibleTo in the Opsgenie docs it's not clear to me whether any of these three values are valid for each type.
- The
usertype goes with eitheridorusername, but not withname - The
teamtype goes with eitheridorname, but not withusername
This validation doesn't take these edge cases into account, could you confirm that this is not an issue?
There was a problem hiding this comment.
It looks like the id and name can be used for any types but username - is only for the user type.
I think it makes sense to add a validation that would make sure that if username is specified then the type must be user. What worries me is that it would cause even bigger drift between alertmanager flavors. If you think this is ok, I will introduce it.
There was a problem hiding this comment.
If the OpsGenie API returns an error, then adding validation at the Alertmanager level is not really a breaking change, but just catching an error before it happens. I'd say double check and go for it.
There was a problem hiding this comment.
It turns out that invalid combinations of the fields are accepted by Opsgenie but ignored. So, I do not think it makes sense to add any additional validation that does not cause the request to fail
There was a problem hiding this comment.
Great, thanks for double-checking :)
| expMsgError: nil, | ||
| }, | ||
| { | ||
| name: "Config with teams responders should be exploded", |
This PR adds support for a new field "responders" to OpsGenie. Documentation https://docs.opsgenie.com/docs/alert-api#create-alert
The config structure and handling logic are taken from vanilla Alertmanager https://github.com/prometheus/alertmanager/blob/9a8d1f976e12b325ec47b84987a78b7845738be6/notify/opsgenie/opsgenie.go#L185-L213
I improved logic around expanding of "teams" responder to make sure that user provides the correct combination of fields.
Can be tested in grafana/grafana#77159