provider/aws: Add support for api_gateway_method_settings

[radeksimko]

Closes #6972
Closes #6612

Test plan

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAPIGatewayMethodSettings_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/04/11 17:21:59 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAPIGatewayMethodSettings_ -timeout 120m
=== RUN   TestAccAWSAPIGatewayMethodSettings_basic
--- PASS: TestAccAWSAPIGatewayMethodSettings_basic (108.34s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/aws	108.374s

[grubernaut]

LGTM, few minor questions

[grubernaut]

There's no difference between Create and Update?

[radeksimko]

There is no create API method, because the method settings is part of the stage which we're updating. The aws_api_gateway_stage resource in turn has no notion of method settings so they shouldn't clash in any way.

[grubernaut]

Do you think they'll/we'll ever allow more than one settings object at a time? If so, might be worth it to go ahead and write a flatten function here. If not, ignore.

[radeksimko]

I doubt it given the nature of these settings (e.g. on/off logging or monitoring) and even if they do so I think we should keep it as is because the ability to use count on top level resources allows the user to do all kinds of things they wouldn't be able to do if they worked with "subresources" (multiple settings blocks nested inside the resource).

[grubernaut]

Just double checking that Value needs to be a string here?

[radeksimko]

It surprised me too when I saw that for the first time, but I guess it makes sense if they want to support patching of whole objects (marshalled JSON which is just string), not just primitive data types.

TL;DR: https://github.com/aws/aws-sdk-go/blob/master/service/apigateway/api.go#L17073-L17077

[apparentlymart]

This sort of resource that updates something that already exists rather than creating a new thing can be confusing. I assume there's a reason not to just add these settings into the api_gateway_method resource itself?

EDIT: oh, I see. It is for a particular method on a particular stage.

[apparentlymart]

Since it seems like this is just being used to construct a prefix for a longer path anyway (and thus method_path is not a first-class concept in the underlying API?), perhaps it would be better to leave these things separated and combine them in code, thus avoiding the need to explain it below as just "concatenate two things together".

[radeksimko]

Thanks for questioning this - I was actually playing with that idea too! 😄

I'm not completely opposed to 2 fields (e.g. path_part & method).

The API allows you to define things like */* or */GET and I'm just little bit worried that they might later introduce (or even already support) some kind of special magic syntax which doesn't involve / and we'd prevent the user from specifying such paths. 🤔

[Porkbutts]

resource.path_part won't work if the resource has a non-root parent though, right? E.g. /a/b/c with method on node c. I tried using resource.path instead, which contains the fully qualified path, but the leading slash returned in the path was causing the settings to not be applied properly. My solution for the time being looks like:

"${substr("${aws_api_gateway_resource.test.path}/${aws_api_gateway_method.test.http_method}", 1, -1)}"

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.