cloudposse / terraform-aws-vpc-peering-multi-account

Terraform module to provision a VPC peering across multiple VPCs in different accounts by using multiple providers
https://cloudposse.com/accelerate
Apache License 2.0
129 stars 92 forks source link

Remove enabled ternary on providers #82

Closed tommij closed 8 months ago

tommij commented 10 months ago

what

removal of enabled part of the for_each ternary, as it disables assume_role on enabled = false, which blocks teardown.

why

aknysh commented 9 months ago

/terratest

aknysh commented 9 months ago

@tommij can you please run the following commands and commit the changes:

make init
make github/init
make readme
nitrocode commented 9 months ago

Just note that this reverts https://github.com/cloudposse/terraform-aws-vpc-peering-multi-account/pull/56 pr. Honestly we need to remove the providers from this module so the root terraform dir that consumes this module can fully control the provider.

See

tommij commented 9 months ago

Just note that this reverts #56 pr. Honestly we need to remove the providers from this module so the root terraform dir that consumes this module can fully control the provider.

See

Not necessarily in disagreement - but I also don't think providers should be be ternary enabled/disabled at any point, as that would effectively mean that you cannot remove resources created with the module. Resources is another thing entirely.

Fortunately, disabled was only the case for assume role parameters, and nothing else - on account of the implementation.

It seems we may have been the only ones using assume_role with role chaining that have tried to set enabled = false with this module, but we did eventually get there :)

aknysh commented 8 months ago

@tommij thank you, please address this:

README.md is outdated. Please run the following commands locally and push the file:
  make init
  make github/init
  make readme

We'll approve the PR since I agree with you that providers should not be disabled in any case (providers are not resources)

aknysh commented 8 months ago

/terratest