config/notifiers: PagerDuty details override defaults

[BenoitKnecht]

According to the Alertmanager documentation, the details option of
pagerduty_config defaults to

[ details: { <string>: <tmpl_string>, ... } | default = {
  firing:       '{{ template "pagerduty.default.instances" .Alerts.Firing }}'
  resolved:     '{{ template "pagerduty.default.instances" .Alerts.Resolved }}'
  num_firing:   '{{ .Alerts.Firing | len }}'
  num_resolved: '{{ .Alerts.Resolved | len }}'
} ]

Which suggests that if details is provided in the configuration, the entire
option is overridden.

But instead, the individual keys of details are being set to the values above
if the corresponding key isn't present in the configuration. This means it's
impossible to remove those keys entirely; setting them to an empty string would
still have them displayed in PagerDuty.

This commit implements the documented behavior, i.e. it sets
PagerDutyConfig.Details to DefaultPagerdutyDetails only if the former is
nil.

[BenoitKnecht]

While I think this corresponds to the documented behavior, I recognize that it would break configuration that relied on the previous implementation.

If you prefer, I can propose a patch that keeps the old behavior but removes the details keys whose values are empty strings, so that it become possible to undefine those default keys.

[levshvarts]

While I think this corresponds to the documented behavior, I recognize that it would break configuration that relied on the previous implementation.

If you prefer, I can propose a patch that keeps the old behavior but removes the details keys whose values are empty strings, so that it become possible to undefine those default keys.

Seems like, from the backwards compatibility point of view, this is the way to go. And even though having an empty string hold a special significance is not super clean, this would not go against any other types of default values in prometheus/alertmanager.

[Preisschild]

Just ran into the same unexpected behavior. Is there a reason why this wasn't reviewed?

[Aaron-ML]

@levshvarts @BenoitKnecht Any chance we can get some eyes on this? Running into this myself and I was able to blank them out in my template however pagerduty stops ordering these fields after exceeding 32 fields total, including blank fields.

[siavashs]

I believe this is not required anymore after #3944
Please open an issue if it still does not behave as expected.

[Aaron-ML]

I believe this is not required anymore after #3944

Please open an issue if it still does not behave as expected.

Was just hitting this problem on latest this week. Will reopen another issue.