Added try to options to avoid errors when enabled is false (#19)

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

Added try to options to avoid errors when enabled is false (#19) #20

Closed JamesTimms closed 3 years ago

JamesTimms commented 4 years ago

what

Added count to accepter and requester options to prevent them from being created when enabled is false.

why

When enabled is false count is set to 0 and is used to skip most resources except these two options. When these options aren't skipped the module fails.

references

closes #19

JamesTimms commented 4 years ago

I added a fix found for a related issue I explained in #19 which only effected Terraform 0.12.11+

JamesTimms commented 4 years ago

@nitrocode @Gowiem Any chance of this being reviewed?

Gowiem commented 4 years ago

@JamesTimms Can you fix your conflict?

Gowiem commented 4 years ago

/test all

JamesTimms commented 4 years ago

@Gowiem I've merged master but saw someone made a similar fix to one of the issues using try(). So I've changed my fix to use try() instead of length() and a ternary. It seems to work fine for me as long as I'm on a version on Terraform that supports try().

nitrocode commented 4 years ago

/test all

JamesTimms commented 4 years ago

@nitrocode & @Gowiem is this ok to merge now or do you need a few more changes?

aknysh commented 4 years ago

@JamesTimms thanks for the PR. Looks like in the final version you removed count = local.count and the final changes are not reflected in the PR description.

I think the count needs to be added anyway to prevent the resources from being created when enabled=false. You final changes are ok, they make TF to not throw errors when data.aws_subnet_ids.accepter.*.ids does not exist.

Please update.

JamesTimms commented 4 years ago

@aknysh, I think someone added the count into a different PR that's made its way into master as the count is still there but not showing up in the diff.

Which lines exactly do you think I've removed count from? The commit here, shows me adding them but then looking at the files (here and here) you can see they're still there despite not showing up in the diff.

JamesTimms commented 4 years ago

Is this likely to get merged?

joe-niland commented 3 years ago

@aknysh I think the count change is not showing in this PR because it was added here: https://github.com/cloudposse/terraform-aws-vpc-peering-multi-account/commit/071e574b62abb4bf9c20820c114096cf25b5cc9e

@JamesTimms given the above, maybe good to update the PR description to match the remaining changes in this PR.

aknysh commented 3 years ago

@aknysh I think the count change is not showing in this PR because it was added here: 071e574

@JamesTimms given the above, maybe good to update the PR description to match the remaining changes in this PR.

Agree with @joe-niland , please update the PR description

JamesTimms commented 3 years ago

@JamesTimms thanks again, just a few nitpicks

I've updated to use empty arrays, including a related change that got into master via some other change. Let me know if this is good enough to merge!

JamesTimms commented 3 years ago

/test all

joe-niland commented 3 years ago

/test all

joe-niland commented 3 years ago

/rebuild-readme

joe-niland commented 3 years ago

/test all

joe-niland commented 3 years ago

Thanks for your contribution @JamesTimms !