search

cloudposse / terraform-aws-security-group

Terraform module to provision an AWS Security Group
https://cloudposse.com/accelerate
Apache License 2.0
36 stars 35 forks source link

change inputs back to string from list(string) #27

Closed mcalhoun closed 2 years ago

mcalhoun commented 2 years ago

what

Nuru commented 2 years ago

/test bats

Nuru commented 2 years ago

/test readme terratest

Nuru commented 2 years ago

@mcalhoun While these changes do avoid the "count" value depends on resource attributes that cannot be determined until apply... error, it looks like they do that by exploiting a bug in Terraform. See https://github.com/hashicorp/terraform/issues/29973

I updated the test to surface the error. The first apply fails, although a second apply will succeed. I don't want to recreate https://github.com/cloudposse/terraform-aws-elasticache-redis/issues/121

Nuru commented 2 years ago

/test all

mcalhoun commented 2 years ago

@mcalhoun While these changes do avoid the "count" value depends on resource attributes that cannot be determined until apply... error, it looks like they do that by exploiting a bug in Terraform. See hashicorp/terraform#29973

I updated the test to surface the error. The first apply fails, although a second apply will succeed. I don't want to recreate cloudposse/terraform-aws-elasticache-redis#121

I see the problem and the issue you opened. I guess we can't use this method until the underlying bug is fixed. Should we ask Hashcorp on that issue what a workaround is instead of turning string into list(string)?

Nuru commented 2 years ago

@mcalhoun said:

I see the problem and the issue you opened. I guess we can't use this method until the underlying bug is fixed. Should we ask Hashcorp on that issue what a workaround is instead of turning string into list(string)?

Turning string into list(string) is Hashicorp's solution to the problem of detecting the presence of optional inputs when the values are unknown. When #29973 is fixed, I expect the fix will be that unknown[*] will throw the same error we get for count and for_each, not that it will magically work the way we want it to.

Nuru commented 2 years ago

@mcalhoun Indeed, as I expected, https://github.com/hashicorp/terraform/issues/29973 has been fixed by ensuring "unknown values upgraded to a collection via a splat expression result in a DynamicVal (unknown value of unknown type), because the resulting type may be a tuple with 0 or 1 elements."

mergify[bot] commented 2 years ago

This pull request is now in conflict. Could you fix it @mcalhoun? 🙏