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

Detector fails to link Slack notification silently #8

Open blkperl opened 6 years ago

blkperl commented 6 years ago

The Slack notification is silently failing. The Pagerduty one is linked but the Slack one is not linked when I look at them in the SignalFX UI.

This is the code that I have that creates the detectors.

resource "signalform_detector" "site_capacity" {
    provider = "signalform"
    count = "${length(var.regions)}"
    name = "${var.regions[count.index]} stack capacity"
    max_delay = 30
    description = "Alerts when stack capacity in a region is full"
    program_text = <<-EOF
        [.....]
        detect("Site Capacity @ 95%", when(site_count > site_capacity * 0.95, lasting='5s'))
        detect("Site Capacity @ 90%", when(site_count > site_capacity * 0.90, lasting='5s'))
    EOF
    rule {
        description = "active when site count exceeds 95% site capacity"
        severity = "Critical"
        detect_label = "Site Capacity @ 95%"
        notifications = ["PagerDuty,RedactedId"]
    }
    rule {
        description = "active when site count exceeds 90% site capacity"
        severity = "Major"
        detect_label = "CDE Site Capacity @ 90%"
        notifications = ["Slack,RedactedId,#channelname"]
    }
}
dichiarafrancesco commented 6 years ago

Hi @blkperl actually we never implemented the slack integration in this provider (L263). I'll make a quick patch.

dichiarafrancesco commented 6 years ago

https://github.com/Yelp/terraform-provider-signalform/pull/9 out for review. I'll update you as soon as it gets merged. I tested it locally. If it keeps failing with an error that looks like the following, the problem is related to invalid credentials:

blkperl commented 6 years ago

Tested #9 and confirmed that it works!

blkperl commented 6 years ago

This could go in a follow up story but I think it should throw an error if the notification type is not defined.

dichiarafrancesco commented 6 years ago

Hi it's a great idea but unfortunately ValidateFunc only works for primitive types so it makes kinda hard to do validations on lists. thanks for the feedback and for taking some time to test my change. really appreciated that ;)

poros commented 6 years ago

It might be possible to add the validation check to https://github.com/Yelp/signalform-tools, since we have already a command to perform more advanced validation of terraform resources: https://github.com/Yelp/signalform-tools/blob/master/signalform_tools/validate.py

We run this validation as git pre-commit hook (https://pre-commit.com/) for our internal repo at Yelp.

@blkperl If this could work for you and you want to take a stab at it, feel free to go ahead.