Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provider/aws: Add support for api_gateway_account #6321

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Apr 24, 2016

I've reached out to Amazon to get a confirmation whether it's really impossible to reset CloudWatch Role ARN to empty string. That's why I marked this as WIP, otherwise it works ok (within the limitations of the API). see below

Test plan

make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSAPIGatewayAccount_basic'
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSAPIGatewayAccount_basic -timeout 120m
=== RUN   TestAccAWSAPIGatewayAccount_basic
--- PASS: TestAccAWSAPIGatewayAccount_basic (61.86s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    61.880s

@radeksimko radeksimko changed the title provider/aws: Add support for api_gateway_account [WIP] provider/aws: Add support for api_gateway_account Apr 24, 2016
@radeksimko
Copy link
Member Author

I received response from AWS support confirming my findings - i.e. it's not possible to remove an ARN or make it blank. 😢

Hi,

From my testing it doesn't look like you can remove this. While this shouldn't be a huge security issue as it's only logging events within that particular APIGW region, I will forward this to our APIGW team to look at as a feature request moving forward.

This is therefore ready for review/merge.

@radeksimko radeksimko changed the title [WIP] provider/aws: Add support for api_gateway_account provider/aws: Add support for api_gateway_account Apr 26, 2016

log.Printf("[INFO] Reading API Gateway Account %s", d.Id())
account, err := conn.GetAccount(&apigateway.GetAccountInput{})
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance that someone deleting an ApiGatewayAccount (using the AWS Console) could cause an error here? Should we check for a 404 and remove from state if so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to delete the "account" even through the AWS Console... so no.

@stack72
Copy link
Contributor

stack72 commented Apr 26, 2016

Few small questions about the PR and 1 nit pick (about adding a test) but that can be fixed later if needed :)

@stack72
Copy link
Contributor

stack72 commented Apr 26, 2016

I guess we can merge this for now and then figure out the leaked resources part later? The tests pass (As below) so this LGTM!

make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSAPIGatewayAccount_basic'
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSAPIGatewayAccount_basic -timeout 120m
=== RUN   TestAccAWSAPIGatewayAccount_basic
--- PASS: TestAccAWSAPIGatewayAccount_basic (57.28s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    57.298s

@radeksimko
Copy link
Member Author

Let me just add two tests (empty resource with "empty" update API call + flattenApiGatewayThrottleSettings) and then you can give it final review & merge.

Hopefully I'll get to it tomorrow.

@stack72
Copy link
Contributor

stack72 commented Apr 26, 2016

👍

@radeksimko radeksimko force-pushed the f-aws-api-gateway-account branch 2 times, most recently from 44a34f8 to dd844e7 Compare April 27, 2016 11:41
@radeksimko
Copy link
Member Author

Both tests added per comments. This is now ready for final review @stack72

@stack72
Copy link
Contributor

stack72 commented Apr 27, 2016

@radeksimko this is awesome :) Thanks!

@stack72 stack72 merged commit e3ade6a into hashicorp:master Apr 27, 2016
@radeksimko radeksimko deleted the f-aws-api-gateway-account branch April 27, 2016 12:22
@ghost
Copy link

ghost commented Apr 26, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants