Yelp / terraform-provider-signalform

SignalForm is a terraform provider to codify SignalFx detectors, charts and dashboards
Apache License 2.0
44 stars 26 forks source link

Treat notification as an object instead of a string #20

Open sjung-stripe opened 6 years ago

sjung-stripe commented 6 years ago

Currently, the notifications associated with a detector rule are stored as a list of strings:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    notifications = ["Email,foo-alerts@bar.com"]
}

I would like to propose storing them as objects instead:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    notification {
        type = "Email"
        email = "foo-alerts@bar.com"
    }
}

or perhaps even:

rule {
    description = "maximum > 60 for 30m"
    severity = "Critical"
    detect_label = "Processing old messages 30m"
    email {
        email = "foo-alerts@bar.com"
    }
}

Motivation for this change: I am planning to add support for the signalfx teams api to signalform, and teams support a notificationlist which (to my understanding) uses the same object as detector rules. It would be useful to have the same structure in both places. In addition, this approach would more closely reflect the underlying API's data model, and would allow for more types of notifications to be supported. (For example, signalform does not currently support sending notifications to a team, which could have its own notification configuration.)

What are your thoughts on this change? I am happy to implement it myself if this is something you'd be willing to accept.

poros commented 6 years ago

This will require quite a bit of refactoring on our side, since we have countless detectors and a couple terraform modules relying on this being a string. @dichiarafrancesco is part of the team maintaining our internal repos, so any non-backward compatible change to the API must be approved by him or someone else on the Metrics Infra team.

Anyway, I think that your change makes sense; I don't know what's the reason why this was implemented as a string in the first place. Not sure which one of the two approaches you proposed is better, though. And team support for SignalFx is something we always had in our backlog, so that is definitely exciting!

dichiarafrancesco commented 6 years ago

Will give it a look on Monday. I need sometime to plan the eventual shift from our current way to-do-things and the new one. hope it's not a big blocker for you.

joshu-stripe commented 6 years ago

Would it be an acceptable workaround if, for the time being, we supported both? We could have notifications as the keyword for the list of strings, and notification as the keyword for the more thorough HCl description.