adding missing failed states for the NAT Gateways

[erutherford]

Should resolve /issues/8687

[kwilczynski]

Would it be possible to re-factor this code a little? I know that you are just adding more checks, but it is getting pretty ugly. We could only do strings.ToLower(*resp.NatGateways[0].State) once, for example. I would also move to using a lookup table to check for many states, but that would be a personal preference.

[erutherford]

Sure, I'll try to get that up later today.

[erutherford]

@kwilczynski I believe that's what you were looking for (I used your changes on the vpc_peering check as a reference, I hope you don't mind). If you'd like to see any additional changes, please let me know.

[kwilczynski]

I would suggest checking for key existence the Go way, for example:

if _, ok := status[strings.ToLower(state)]; ngRaw == nil || ok {

[erutherford]

Ok, I'll re-work this and re-submit.

[kwilczynski]

@erutherford thank you so much! Does this solve the problem you are seeing and does the acceptance test pass?

[erutherford]

@kwilczynski unfortunately, I was able to get out of my broken state by tainting the NAT Gateway resources and forcing the recreation.

I can try to see if I can reproduce the issue by running an apply while the NAT gateway is in the deleting state, but reproducing the Internet Gateway timeout that occurred originally may be difficult. I'm open to suggestions though.

On the acceptance testing front. Sorry, I skimped on that and had assumed it was a part of the CI testing that was occurring. I'll be sure to run that on my next commit before submitting it. Thanks again for your guidance.

[kwilczynski]

@erutherford the CI tests don't run acceptance tests (this happens once a day at the moment). You can run these by looking at the name of the tests and then invoking make testacc with appropriate arguments, for example (from some other commit):

make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEFSFileSystem'

I would recommend exporting the AWS credentials as environment variables so that Terraform would pick these up.

[erutherford]

@kwilczynski I've updated to the format you mentioned and I've ensured the acceptance testing is passing for TestAccAWSNatGateway. Sorry for all of the churn on this, thanks again for the help.

[kwilczynski]

@erutherford no, thank you! :)

You've done a remarkable thing: found an issue, took the time to troubleshoot, lodged an issue, and sent a fix. I wish everyone was doing this! Thank you so much for your help!

Over to @stack72 now 🚀

[stack72]

Hi @erutherford

To echo the statement that @kwilczynski made - a huge thanks for the contribution here :) It's great to get bug reports, but it's even better when someone helps us by fixing it as well :)

I am just running the tests here and will merge when green

Thanks

Paul

[stack72]

FYI:

% make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSNatGateway'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/09/07 09:52:47 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSNatGateway -timeout 120m
=== RUN   TestAccAWSNatGateway_importBasic
--- PASS: TestAccAWSNatGateway_importBasic (261.46s)
=== RUN   TestAccAWSNatGateway_basic
--- PASS: TestAccAWSNatGateway_basic (264.03s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    525.518s

[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.