set enabled for codebuild module

[mihaiplesa]

what

• currently CodePipeline resources are created even if we set enabled to false

why

• we'd like to not create CodePipeline resources when enabled is false

references

• closes Resources created when enabled set to false #101

[joe-niland]

/test all

[mihaiplesa]

@joe-niland can we try tests again?

[joe-niland]

/test all

[mihaiplesa]

Ok, should work now @joe-niland.

[joe-niland]

/test all

[joe-niland]

@mihaiplesa let's check that, but I think the test is going to keep failing due to duplicate bucket name. I'm looking at fixing the test now.

[mihaiplesa]

I've tested with cloudposse/terraform-aws-ecs-web-app@master...mihaiplesa:terraform-aws-ecs-web-app:master and setting codebuild_enabled to false.

One last possible change would be to replace [0] with .*..

[joe-niland]

/test terratest

[mihaiplesa]

Ugh, for using aws_s3_bucket_acl we need AWS provider v4 and this module has >= 2.0 so we'd break compatibility if we merge this. Similar in https://github.com/cloudposse/terraform-aws-codebuild.

Can we just merge without aws_s3_bucket_acl if the only broken tests are because of the deprecation warning?

[joe-niland]

@mihaiplesa yes, we can leave the deprecation warning in for this one. If you want to revert that, let's try again.

[mihaiplesa]

@joe-niland done.

[joe-niland]

/test all

[joe-niland]

@mihaiplesa all your issues are OK now, but the tests are still failing due to a bucket name collision. I'm fixing that in this PR but needs an approval before merging.

[joe-niland]

@mihaiplesa can you please merge master and then we'll try it again?

[mihaiplesa]

@joe-niland done.

[joe-niland]

/test terratest

[joe-niland]

/test bats

[joe-niland]

/test readme

[joe-niland]

Thanks @mihaiplesa we got there in the end!

[mihaiplesa]

Thank you as well. This is where it will actually be used cloudposse/terraform-aws-ecs-web-app#210

[nitrocode]

@joe-niland This change seems like a no-op.

For future ref: The change here passed in enabled = module.this.enabled but this is already passed in via the context.

https://github.com/mihaiplesa/terraform-aws-ecs-codepipeline/blob/f2b43ffedc30a7f4f7f57b59f9020b79fb3f8a83/main.tf#L239

@mihaiplesa please use the PR template.

If the above functionality does not have the intended effect without applying this change, then something else is very wrong...

[joe-niland]

@nitrocode thanks, I can see that now.

@mihaiplesa I should have asked you which codebuild resources were created when enabled == false. Could you let us know if you still see the original issue?

[mihaiplesa]

Hmm I can't reproduce now which seems to be reassuring, it's been a while since this issue has been on my mind so I might be mixing things.

Also noticed now that module.this.enabled if only passed where there is a count or a more complex conditional.

Do we want to revert this PR?