feat(alerts): add notify_on_acknwoledge, deprecated opts at notification channel level

[dbonf]

• add new attribute notify_on_acknwoledge on all alert resources, it should be used if one want to send a notification for an alert that has been manually acknowledged by a user;
• deprecate notify_when_ok and notify_when_resolved on notification channel resources: users should define these behaviors at alert level now.

technical note: to allow for boolean attributes with no default, we use the string type, enforcing "true" and "false" values, instead of boolean values, because with the current terraform sdk there is no way to distinguish "false" from no value defined.

[tembleking]

I understand the reasoning behind using TypeString to distinguish between false and "not defined", since this is a known limitation of the Terraform SDK v2.

However, I have two concerns:

1. For the new notify_on_acknowledge field: Using TypeString makes sense here since it's a new field and you need the three-state behavior (true/false/unset). But what's the meaning of not setting it? Isn't it the same as define it as false?
2. For the existing notify_when_ok and notify_when_resolved fields: Changing from TypeBool with Default: false to TypeString without a default is a breaking change. Even though HCL accepts unquoted true/false as strings, the state type changes and the default behavior changes (users relying on the implicit false default will now get nil). Since these fields are being deprecated anyway, I'd suggest keeping them as TypeBool with their current behavior because this avoids breaking existing configurations while still deprecating the fields.

[dbonf]

I understand the reasoning behind using TypeString to distinguish between false and "not defined", since this is a known limitation of the Terraform SDK v2.

However, I have two concerns:

1. For the new notify_on_acknowledge field: Using TypeString makes sense here since it's a new field and you need the three-state behavior (true/false/unset). But what's the meaning of not setting it? Isn't it the same as define it as false?

as mentioned in the doc, the behaviour on missing definition of this field at alert level is to take the setting at notification channel level. In the future, when we make it optional at nc level and/or remove it from there, the behavior is to send a notification in this case.

2. For the existing notify_when_ok and notify_when_resolved fields: Changing from TypeBool with Default: false to TypeString without a default is a breaking change. Even though HCL accepts unquoted true/false as strings, the state type changes and the default behavior changes (users relying on the implicit false default will now get nil). Since these fields are being deprecated anyway, I'd suggest keeping them as TypeBool with their current behavior because this avoids breaking existing configurations while still deprecating the fields.

i agree there can be scenarios where the behavior change. We will need a breaking change anyway in the near future because we need to remove this from the ui. Because of this, I agreed to leave the fields as is, default: false, and add the deprecation notice. Then in the near future we will have to apply back this breaking change on default change or remove the fields altogether with a new major version release.

[tembleking]

LGTM

[tembleking]

as mentioned in the doc, the behaviour on missing definition of this field at alert level is to take the setting at notification channel level. In the future, when we make it optional at nc level and/or remove it from there, the behavior is to send a notification in this case.

Understood, then it makes sense

i agree there can be scenarios where the behavior change. We will need a breaking change anyway in the near future because we need to remove this from the ui. Because of this, I agreed to leave the fields as is, default: false, and add the deprecation notice. Then in the near future we will have to apply back this breaking change on default change or remove the fields altogether with a new major version release.

Yeah, let's keep it as compatible as possible so people can migrate to the new field and we will remove those after some time.

Thank you for the submission @dbonf