-
-
Notifications
You must be signed in to change notification settings - Fork 633
fix: Incorrect VPC selection for Security group introduced in #353 #357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| data "aws_subnet" "this" { | ||
| count = local.create_security_group && var.vpc_id != null ? 1 : 0 | ||
| count = local.create_security_group ? 1 : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incorrect - it should be
| count = local.create_security_group ? 1 : 0 | |
| count = local.create_security_group && var.vpc_id == null ? 1 : 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryantbiggs, with the suggested code, the Terraform plan is failing on example/complete. I tried this as well, but it didn't work, and I decided not to investigate further why it's not working.
│ Error: Invalid count argument
│
│ on ../../modules/service/main.tf line 1641, in data "aws_subnet" "this":
│ 1641: count = local.create_security_group && var.vpc_id == null ? 1 : 0
│
│ The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many
│ instances will be created. To work around this, use the -target argument to first apply only the resources that the count
│ depends on.
| name_prefix = var.security_group_use_name_prefix ? "${local.security_group_name}-" : null | ||
| description = var.security_group_description | ||
| vpc_id = try(data.aws_subnet.this[0].vpc_id, var.vpc_id) | ||
| vpc_id = coalesce(var.vpc_id, data.aws_subnet.this[0].vpc_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is incorrect - it has the potential to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bryantbiggs, do you have any ideas on how to prevent this?
|
This issue has been resolved in version 6.6.1 🎉 |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
Fixes incorrect VPC selection introduced in #353
Motivation and Context
Discussions can be found here and here
Breaking Changes
No
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request