feat: default tgw route table tags (rebased against current HEAD)#49
Conversation
main.tf
Outdated
| } | ||
|
|
||
| resource "aws_ec2_tag" "this" { | ||
| count = var.create_tgw ? length(local.tgw_default_route_table_tags_merged) : 0 |
There was a problem hiding this comment.
for_each is generally a better option, because with count it will recreate tags when the order changes.
We should aim to use for_each for new features (like this one).
There was a problem hiding this comment.
agreed, however that would require making this resource required as opposed to optional. Since count and for_each are mutually exclusive. Making this non-optional potentially opens to door to failed runs, since aws_ec2_transit_gateway.this[0].association_default_route_table_id is a dependency on of the resource, if aws_ec2_transit_gateway.this isn't created because someone passed the create_tgw the module run would also fail.
IMO If we want to do that we should refactor and remove the create_tgw parameter entirely. But doing that seemed like it was outside the scope of this change.
There was a problem hiding this comment.
We can write for_each to be conditional, too. For example, https://github.com/terraform-aws-modules/terraform-aws-eventbridge/blob/master/main.tf#L166
create_tgw is important to control creation when calling this module from Terragrunt, for example.
There was a problem hiding this comment.
Hmm yeah that could work, let me add it to this branch and re-push!
There was a problem hiding this comment.
nice, that worked well! Just pushed an update!
|
v2.3.0 has been just released. |
|
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. |
3 participants
Description
This is this same PR #34 just rebased against current master
Motivation and Context
This is a great feature, and IMO much needed
Breaking Changes
This change doesn't break compatibility, however since it uses
countany change to the tags will generate a bit of resource churn. It might be worth discussing how desirable thecreate_tgwparameter is in later releases and if we should keep it around.How Has This Been Tested?
Tested against my local dev env.
examples/*projects