Modify alert_channel to use maps for headers, payload

[aaronshaver]

A possible solution for #66 - this change:

• un/marshalls headers and payload into separate maps
• tests various configurations of these optional fields
• documents these fields with examples

[t0astbandit]

@paultyng when you have some time would you please review this PR?

[paultyng]

Sorry for the delay on this, things are taking longer as we ship 0.12. I left a few comments regarding backwards compatibility. Maybe we should name these fields webhook_headers and webhook_payload just to be more explicit that they don't apply to the other types? Should we throw an error if they have data and its not a webhook?

[paultyng]

I'm a little hesitant to do this delete as its a breaking change for anyone already using this behavior.

[nathanael-ness]

I do not believe that the delete could break anything because, as far as I can tell setting the headers or payload is broken currently

[brasey]

I think nathanael-ness is right, I don't see how either headers or payload could be working currently with the type mismatch.

We're having to use a null-resource to deploy a webhook alert channel, can't use the resource as it is.

[paultyng]

I'm a little hesitant to do this delete as its a breaking change for anyone already using this behavior.

[brasey]

Also related: #45

[brasey]

And #18

[StephenWeber]

Having reviewed the associated issues and thinking about how the code operates, I wanted to re-summarize the design. For most of it I'll talk about payload but all commentary is applicable to headers as well.

This PR adds the ability to specify the payload and headers as maps. This PR's proposed HCL example:

alert_channel mychannel {

    configuration {
    	... other things ...
    }

    headers = {
    	api-key = "my-internal-api-key"
    }
    payload = {
    	account_id = "test"
    }

}

The comment raised by @paultyng seems like people may be doing this already to get through the API:

alert_channel {

    configuration {
    	headers = "{ 'header': 'value' }"
    	payload = "account_id=test"
    }

}

It's possible some users with simple headers are providing an explicit string in alert_channel.configuration.payload. If so, we shouldn't just clobber those in favor of the map-style. Reconciling these two methods raises two situations:

1. User specified payload (or header) in one place, there is no conflict. We use whichever field they provided (alert_channel.configuration.payload or alert_channel.payload) - we should support both methods and do the right thing.
2. User specifies payload in both alert_channel.configuration.payload or alert_channel.payload and there's a conflict

We should catch the situation where the user mistakenly sets both fields, that may be in conflict, and issue a warning that helps clarify what they should do. Perhaps we change the field names to be more clear about this, such as payload_map, and still set it in the API's config as we are here.

So I think the proposal for updating this PR is:

1. changing the (optional) map field to be named payload_map
2. checking if a string is set at alert_channel.configuration.payload and using that
3. checking if both fields are specified and issuing a warning about the conflict
4. probably fixing the lines that delete payload from the configuration block to respect which place in HCL the payload is stored
5. updating / adding tests to cover the new design
6. updating documentation and examples to reflect config options

Anything I've missed or that should also be considered?

[beardface]

This issue has been open for a while now, does anyone have a work around?

[ctrombley]

Related to paultyng/go-newrelic#52

[ctrombley]

The terraform docs here explain how to use the ConflictsWith schema attribute to implement the either/or checking @StephenWeber describes above.

[ctrombley]

Closing this in favor of the implementation in #307.