Fix module disabled still creates infrastructure

paulrob-100 commented 3 years ago

what

Module should not create infrastructure if enabled = false
Fixed test failing due to tgw apply dependency on the vpc/subnets apply
Added new test/fixtures for enabled/disabled variables
Base vpc infrastructure should always be enabled
Added an optimisation for slow tests
Fixed code so that the new test passes

why

Bug fix - see #10
I encountered issues running the go test that also needed fixing to submit the PR
The tests are very slow to run, so introduced a slight optimisation around the destroy

references

closes #10

maximmi commented 3 years ago

/test-all

maximmi commented 3 years ago

/test all

paulrob-100 commented 3 years ago

Summary of failing tests

TestExamplesComplete

 TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: Error: error creating Route in Route Table (rtb-0f9d9bfd03632171b) with destination (172.48.0.0/16): InvalidTransitGatewayID.NotFound: The transitGateway ID 'tgw-0628808dbf1f9591a' does not exist.
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   status code: 400, request id: 57eda4d3-221c-468e-b635-66a14fa8443d
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   on ../../modules/subnet_route/main.tf line 14, in resource "aws_route" "count":
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   14: resource "aws_route" "count" {
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: Error: error creating Route in Route Table (rtb-0efab8c02c2010cb1) with destination (172.48.0.0/16): InvalidTransitGatewayID.NotFound: The transitGateway ID 'tgw-0628808dbf1f9591a' does not exist.
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   status code: 400, request id: 57c41824-00b3-4561-9d1d-fa1e51573e85
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   on ../../modules/subnet_route/main.tf line 14, in resource "aws_route" "count":
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   14: resource "aws_route" "count" {
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: Error: error creating Route in Route Table (rtb-0efdd31ddba9460f3) with destination (172.48.0.0/16): InvalidTransitGatewayID.NotFound: The transitGateway ID 'tgw-0628808dbf1f9591a' does not exist.
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   status code: 400, request id: a526ce5b-7412-4ce1-9297-9da7795920d7
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121: 
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   on ../../modules/subnet_route/main.tf line 14, in resource "aws_route" "count":
TestExamplesComplete 2021-07-28T08:50:58Z command.go:121:   14: resource "aws_route" "count" {

I have seen this in my local tests also. When you check the AWS console, you'll find that the tgw-0628808dbf1f9591a does exist but the API calls don't seem to find it. It seems to be related to this one https://github.com/hashicorp/terraform-provider-aws/issues/10025 which has never been fixed. There's nothing much I can do to fix this I'm afraid. Rerunning is the only option without a fix in the AWS provider.

Note: While iterating on the tests, I've also seen my local test hit the limit for the number of transit gateways per region. It's one which can be raised if necessary.

TestExamplesCompleteDisabledModule

    examples_complete_test.go:136: 
            Error Trace:    examples_complete_test.go:136
            Error:          Not equal: 
                            expected: "\"\""
                            actual  : ""

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -""
                            +
            Test:           TestExamplesCompleteDisabledModule
    examples_complete_test.go:137: 
            Error Trace:    examples_complete_test.go:137
            Error:          Not equal: 
                            expected: "\"\""
                            actual  : ""

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1 +1 @@
                            -""
                            +
            Test:           TestExamplesCompleteDisabledModule

I found I had to use assert.Equal with quoted double quotes to get my tests to pass locally. I originally used assert.Empty.

Perhaps there's a difference in the terratest version, although I'm using the go module to download it. Checking.

github.com/gruntwork-io/terratest v0.16.0

maximmi commented 3 years ago

@paulrob-100 please, update terratest to at least v0.31.1 here: https://github.com/cloudposse/terraform-aws-transit-gateway/blob/master/test/src/go.mod#L9 and I will re-run tests

paulrob-100 commented 3 years ago

Thanks for your review @maximmi.

I'll attempt the terratest upgrade but noticed there have been many breaking changes since 2019. I noted from the release notes that later versions are only tested with certain versions of TF. https://github.com/gruntwork-io/terratest/releases?after=v0.18.1

v0.31.0 does indeed solve my issue with the local tests however. https://github.com/gruntwork-io/terratest/releases/tag/v0.31.0

All code changes should be backwards compatible, but marking this release as incompatible because from this release onwards, we are only testing with Terraform 0.14 and up.

Note: the only difference I can see between the test-harness container and running locally is the tests are choosing TF 0.13. I can see that the actual terratest step is running go mod download as expected. I also checked several other cloudposse modules and they're all using 0.16.0

Terraform 0.13 is required for master...
GOLANG_VERSION=1.15.11
go mod download
go test -v -timeout 60m -run TestExamplesComplete

I think this also means the test harness terraform version needs to be bumped to at least 0.14.

paulrob-100 commented 3 years ago

Pushed those fixes @maximmi . I didn't encounter any issues with the bump in my local testing.

aknysh commented 3 years ago

/rebuild-readme

paulrob-100 commented 3 years ago

thanks for your review @aknysh :raised_hands:

I ran the make targets as described, but there were no changes outside of the .github directory. The readme and terraform fmt were automatically fixed in fa78dac95 by the github action.

If you feel you need the changes in the .github directory, you should be able to push those to my branch.

Note: I'll be away from my desk for a few days, so I'll pick this up again next week.

paulrob-100 commented 3 years ago

good morning @aknysh, is there anything outstanding on this?

I'm unfamiliar with your github actions configuration, so I'm unsure if you prefer me to commit the changes to the .github directory after the make file downloads?

nitrocode commented 3 years ago

/test all

paulrob-100 commented 3 years ago

Tests failed for some reason as described before https://github.com/cloudposse/terraform-aws-transit-gateway/pull/11#issuecomment-888199952

A potential workaround could be to restructure the test into 2 module calls, the first just creating the tgw attachments and the second module call to create the routing table.

The other problem has been fixed by bumping the terratest version.

paulrob-100 commented 3 years ago

Pushed attempted fix after inspiration from https://github.com/hashicorp/terraform-provider-aws/issues/13830#issuecomment-713145404

korenyoni commented 3 years ago

/test all

nitrocode commented 3 years ago

/test all

paulrob-100 commented 3 years ago

@nitrocode the tests are passing now, so hopefully robust going forward :+1:

There were some earlier comments still unresolved. Do you have any preferences there?

If not, please can this be merged?

paulrob-100 commented 3 years ago

@nitrocode just a bump on this one please. :pray:

nitrocode commented 3 years ago

Thanks for the ping. I haven't heard back yet @paulrob-100 and we've all been very busy with work. We'll review when time permits.

cc: @aknysh

mergify[bot] commented 3 years ago

This pull request is now in conflict. Could you fix it @paulrob-100? 🙏

nitrocode commented 3 years ago

/test all

paulrob-100 commented 3 years ago

@nitrocode I've pushed a new commit and also added a couple of new PRs which are rebased on this one.

I assume this PR will be merged, but hoping there might be some time for the final review. I appreciate you're busy, as am I, but the PRs are all there for the community if desired. :+1:

nitrocode commented 3 years ago

/test all

nitrocode commented 3 years ago

Thanks Paul for the contribution and working with us to get this merged.

paulrob-100 commented 3 years ago

Excellent thank you for your review @nitrocode @aknysh

cloudposse / terraform-aws-transit-gateway