fix: Change count for for_each on ram_principal_association due resource recreation #95
Conversation
ebarros29
changed the title
|
This PR has been automatically marked as stale because it has been open 30 days |
…e and adjust the output related to this resource
ebarros29
force-pushed
the
fix/avoid-recreate-ram-principal-association
branch
from
e56038e to
f4660da
Compare
|
Folks! |
|
can we remove the stale label? |
|
@antonbabenko @bryantbiggs |
Yes, it is! What kind of issues we can expect different from what has been described into PR? |
main.tf
Outdated
| @@ -161,9 +161,9 @@ resource "aws_ram_resource_association" "this" { | |||
| } | |||
|
|
|||
| resource "aws_ram_principal_association" "this" { | |||
| count = var.create_tgw && var.share_tgw ? length(var.ram_principals) : 0 | |||
| for_each = var.create_tgw && var.share_tgw ? toset(var.ram_principals) : [] | |||
There was a problem hiding this comment.
if any values provided in var.ram_principals are computed, this will fail. in addition, the left side of the conditional is of type toset() and the right is tolist()
There was a problem hiding this comment.
if any values provided in
var.ram_principalsare computed, this will fail.
You have a good point! I will fix it by using a local value.
in addition, the left side of the conditional is of type
toset()and the right istolist()
Why tolist()? The variable ram_principals is already a list, moreover for_each cannot iterate over lists as you know and that was the reason I used toset()... Did I miss something?
| @@ -1,4 +1,7 @@ | |||
| locals { | |||
| # Store the list of principals and convert to set | |||
| ram_principals = var.create_tgw && var.share_tgw ? toset(var.ram_principals) : [] | |||
There was a problem hiding this comment.
simply moving this logic to a local does not correct its flaws
again, lets look at the conditional where its generally of the format conditional ? true case : false case
- When the conditional is true, the type is of
toset() - When the conditional is false, the type is of list since its just empty brackets
This is an issue because the types are now mis-matched (toset() vs list).
In addition, because you are using toset() on a list, any of the values in that list cannot be computed or else you will face the well known issue of not being able to use for_each on a value that is not yet known.
Lastly, changing from count to for_each is a breaking change
There was a problem hiding this comment.
I guess what @bryantbiggs is trying to say is since the left side of the conditional is a set you need the right one to be set too, i.e. toset([]) instead of [] which is of type list.
There was a problem hiding this comment.
simply moving this logic to a local does not correct its flaws
again, lets look at the conditional where its generally of the format
conditional ? true case : false case
- When the conditional is true, the type is of
toset()- When the conditional is false, the type is of list since its just empty brackets
This is an issue because the types are now mis-matched (
toset()vs list).In addition, because you are using
toset()on a list, any of the values in that list cannot be computed or else you will face the well known issue of not being able to usefor_eachon a value that is not yet known.Lastly, changing from count to for_each is a breaking change
Sorry, Bryan!
I thought you were asking to use tolist() function insted toset() 😅
There was a misunderstanding on my part and now I got it!
Indeed, for_each cannot predict these values (when computed) while using toset() function , neather iterate over a list.
I agree with u, it’s a breaking change and maybe it’s better keep this code as is, right?
There was a problem hiding this comment.
I guess what @bryantbiggs is trying to say is since the left side of the conditional is a
setyou need the right one to be set too, i.e.toset([])instead of[]which is of typelist.
Exactly, Igor! Thanks 🙇
However, even solving this part (making the sides equal) we will face that for_each known issue mentioned by Bryan... 😢
There was a problem hiding this comment.
I was take a looking on main.tf again and currently it uses count on length(var.ram_principals) here:
terraform-aws-transit-gateway/main.tf
Line 165 in f4db667
How count will know the length of the list for computed values? In this case we wil face the same problem (unknown value), right?
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Just to remove stale label... |
|
Maybe an approach could be to write a local module for the association and then use for_each to instantiate many instances of the module? |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
keep alive |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Any chance to have this done? (keep alive) |
|
Sorry guys, It has been busy days since my last comment 😫 Actually I don't know if it's worth to go ahead with this PR, since we have this huge refactor #113 already marked as |
If it manages this issue then it should be ok to close this PR. Otherwise I'd go ahead and the refactor will pick the fix from this PR as well. |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
"Keep alive" post |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
"Keep alive" post |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
"Keep alive" post |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
"Keep alive" post |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
no chance to have this worked? (KA :P) |
|
"Keep alive" post |
|
Maybe revert the breaking change first ( going back to |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
This PR was automatically closed because of stale in 10 days |
|
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
Change
countforfor_eachonaws_ram_principal_associationdue resource recreation.Motivation and Context
In order to avoid recreation of the resource
aws_ram_principal_associationeverytime that a principal is removed or inserted in the middle of the list, it's necessary to changecountforfor_eachloop.Breaking Changes
The meta-argument
countuse indices to organize the list, for instanceaws_ram_principal_association.this["0"].The meta-argument
for_eachuses "key, value" to organize it, for instanceaws_ram_principal_association.this["01234567890123"]where01234567890123will be the principal account ID for this case.So due these differences it will cause a recreation of all existing
aws_ram_principal_associationresources and the people who use RAM for share the TGW will need to runterraform state mvin order to avoid this recreation.How Has This Been Tested?
It has been tested using the module with input
share_tgw = trueby analysing theplanand outputs afterapply.examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request