terraform-aws-ecs-codepipeline icon indicating copy to clipboard operation
terraform-aws-ecs-codepipeline copied to clipboard

Published 20 hours ago • verified publisher icon cloudposse

set enabled for codebuild module

Open mihaiplesa opened this issue 2 years ago • 13 comments

Resolves https://github.com/cloudposse/terraform-aws-ecs-codepipeline/issues/101

mihaiplesa avatar Oct 14 '22 06:10 mihaiplesa

/test all

joe-niland avatar Oct 14 '22 06:10 joe-niland

@joe-niland can we try tests again?

mihaiplesa avatar Oct 14 '22 07:10 mihaiplesa

/test all

joe-niland avatar Oct 14 '22 07:10 joe-niland

Ok, should work now @joe-niland.

mihaiplesa avatar Oct 14 '22 07:10 mihaiplesa

/test all

joe-niland avatar Oct 14 '22 08:10 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.

joe-niland avatar Oct 14 '22 08:10 joe-niland

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

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

mihaiplesa avatar Oct 14 '22 08:10 mihaiplesa

/test terratest

joe-niland avatar Oct 14 '22 08:10 joe-niland

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?

mihaiplesa avatar Oct 14 '22 08:10 mihaiplesa

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

joe-niland avatar Oct 14 '22 08:10 joe-niland

@joe-niland done.

mihaiplesa avatar Oct 14 '22 08:10 mihaiplesa

/test all

joe-niland avatar Oct 14 '22 09:10 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 avatar Oct 14 '22 10:10 joe-niland

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

joe-niland avatar Oct 19 '22 06:10 joe-niland

@joe-niland done.

mihaiplesa avatar Oct 19 '22 07:10 mihaiplesa

/test terratest

joe-niland avatar Oct 19 '22 07:10 joe-niland

/test bats

joe-niland avatar Oct 19 '22 07:10 joe-niland

/test readme

joe-niland avatar Oct 19 '22 07:10 joe-niland

Thanks @mihaiplesa we got there in the end!

joe-niland avatar Oct 19 '22 08:10 joe-niland

Thank you as well. This is where it will actually be used https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/210

mihaiplesa avatar Oct 19 '22 13:10 mihaiplesa

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

nitrocode avatar Oct 19 '22 15:10 nitrocode

@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?

joe-niland avatar Oct 19 '22 21:10 joe-niland

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?

mihaiplesa avatar Oct 20 '22 03:10 mihaiplesa