Add resource for integrations

[sjung-stripe]

This resource wraps the signalfx integration endpoint. Right now it only supports slack and pagerduty (the ones we use) but adding more should be pretty simple. I added corresponding documentation; let me know if it's inadequate or lacking in some way.

[poros]

Would it be a good usability improvement to make this check less strict so also "pagerduty" and "Pagerduty" would match the condition?

[sjung-stripe]

I think SFX's own API might actually be case sensitive on this value, so exact match seems correct to me.

[poros]

Oh sorry, my fault, that was poorly phrased.
I meant that it would be nice to give users a bit more of flexibility so they are not confused by errors when applying terraform just because of some casing issue. If you accept any casing from the user and then you transform the input into the right form (e.g. Pagerduty -> PagerDuty), I think this would deliver a much better user experience.

It's just a suggestion, though, not an hard blocker at all.

[poros]

I think that it would be worth mentioning in the documentation that users must have a token with admin rights in order to be able to edit integrations.

[sjung-stripe]

good catch, i'll add that

[poros]

It's a supercool addition! @sjung-stripe ++
As usual, I'll defer to @dichiarafrancesco for the real review, but I personally really like it

[sjung-stripe]

ping on this, any other changes requested here?

[rahulravindran0108]

Is there a reason we are not validating the presence of api_key for PagerDuty and webhook_url for slack ? The check here is only done for integration type but I think it should also perform other necessary checks.

[sjung-stripe]

The validation function can only check a single property at a time, so cross-property checks ("if this resource has type=pagerduty, then it must have an apikey") cannot be represented using validation functions. The only way to implement this check (that I can see) is in getPayloadIntegration. I suppose it's fine to add validation there, but signalfx itself will also do that validation on the server side immediately after the result of getPayloadIntegration is POSTed to their api, so I don't think there's much safety added by validating in the provider.

[rahulravindran0108]

@sjung-stripe sounds good. I think we can still ensure some validation in the schema itself with ConflictsWith. This is not really a blocker, but would be nice to have one level of protection here itself.

Also, another issue is if we are to make this extendible to other integration, this schema might become a problem. We might have to end up modelling nested schema here for different integration with all the optional fields out there.

Ref (ConflictsWith): https://github.com/Yelp/terraform-provider-signalform/blob/master/src/terraform-provider-signalform/signalform/detector.go#L75

[sjung-stripe]

Also, another issue is if we are to make this extendible to other integration, this schema might become a problem. We might have to end up modelling nested schema here for different integration with all the optional fields out there.

I wanted to do something like this, but wasn't able to represent it in the schema. Do you have something concrete in mind?

re conflictswith - sure, I can add that.

[rahulravindran0108]

Yes we could do something like:

"slack": &schema.Schema{
    Type:        schema.TypeSet,
    Optional:    true,
    Description: "slack integration",
    ConflictsWith: (all other named integration here)
    Elem: ... (whatever keys you want),
"pagerduty": .....(same as above)

In this case each integration now becomes a key. It will be optional, conflictWith other integrations ensuring there is only one integration type is defined. Further, you can now make the attributes such as webhook_url or api_key required within the integrations

[sjung-stripe]

The problem with that approach is that it's using schema.TypeSet, which means this is now legal:

resource "signalform_integration" "slack" {
  name = "foo"
  enabled = true
}

ie you can specify zero copies of everything. You can't fix this with ValidateFunc either, because you'd have to check each of the keys (slack, pagerduty, etc) and make sure at least one was nonzero.

[rahulravindran0108]

I'm sorry I didn't quite understand your previous explanation.

I was thinking of including "slack" like this (axis_right) https://github.com/Yelp/terraform-provider-signalform/blob/master/src/terraform-provider-signalform/signalform/time_chart.go#L115 not as a resource.

You can define the elemTypes you need now and make them required. Further, make slack conflictWith the other integration types you define.

[sjung-stripe]

Yes, the example you indicated has the same problem I'm describing. It's using a TypeSet which means it can be specified zero or more times. All of the following would be legal:

resource "signalform_integration "slack" {
  name = "foo"
  enabled = true
  slack {
    webhook_url = "foo"
  }
}

resource "signalform_integration "slack" {
  name = "foo"
  enabled = true
}

resource "signalform_integration "slack" {
  name = "foo"
  enabled = true
  slack {
    webhook_url = "foo"
  }
  slack {
    webhook_url = "bar"
  }
}

This is why I didn't use typed keys to begin with - because the only way to do so is to specify them on an Elem, which requires using a map, list, or set.

[rahulravindran0108]

Aah! understood what you meant now, Sorry about that.

The project structure more or less follows this structure elsewhere (the one I mentioned above of using TypeSet). I feel that way, the structure is more appropriate than it currently is, considering the number of integrations out there and most of the attributes vary.

signalform-tools is another repository we maintain for validation and we can definitely bake in the functionality there.

This is not really a blocker, feel free to disregard.