#pr-reviews (2023-12)
Pull Request Reviews for Cloud Posse Projects
2023-12-01
May I ask about code review once again? The same pull request as before. This feature is really important for me and I would like to upgrade my redis clusters to 7.1. Also, as you can see other people also are waiting for it (please note that there are a few pull requests about the same feature)
@Jeremy White (Cloud Posse)
@Andriy Knysh (Cloud Posse)
regarding your question “I would like to ask: what is the advantage of creating a new local variable in this case?” - b/c it’s used in two diff places
yes, currently it’s used in two diff places, but before it was only in one place - this is why I asked Currently, it uses a local variable
before I asked you to update the description
as well
since it uses the old logic for param group name
I’ll ask you to do the same again
ok
@Andriy Knysh (Cloud Posse) thank you code review! May I have one more question about local.enabled
vs module.this.enabled
I see that:
locals {
enabled = module.this.enabled
...
}
so in this case local.enabled
adds only more complexity I think.
local.enabled
is used in all modules for 1) consistency; 2) to not repeat the expression module.this.enabled
in the code b/c local.enabled
could use some other logic to make all resources enabled or disabled; and 3) the mdule already defined local.enabled
and uses it, so why not use it again
I see, but there is no additional logic here, also local.enabled
is not used in the code
there is module.this.enabled
everywhere
of course I can change it, n/p, just wonder
not used? Then it’s not good Should be fixed
all our modules use local.enabled
for the reasons explained above
if it’s defined and not used anywhere in the code, then it’s a declared and unused variable, which is a “code smell”
I see what you are saying, the code has a bunch of count = module.this.enabled ? 1 : 0
can I ask you to fix that as well? (search for module.this.enabled
everywhere and replace it with locl.enabled
)
sure
I can replace module.this.enabled
to local.enabled
Done
thanks
please see this error https://github.com/cloudposse/actions/actions/runs/7062118033/job/19225231599
family = "redis6.x"
in the PR description, you have “Since using .
is not possible, I have opted to use -
instead”, but that’s not in the code - please replace .
with -
good point! I was testing with redis7
. Let me fix it
fixed
Thank you for your help!
Thanks @Andriy Knysh (Cloud Posse) !!
2023-12-21
May I ask for a code review for this pull request: https://github.com/cloudposse/terraform-aws-elasticache-redis/pull/217
There is a bug in the terraform aws provider: hashicorp/terraform-provider-aws#32835
When importing an aws_elasticache_replication_group
resource the attribute security_group_names
is imported as null
Currently, we can’t import existing aws_elasticache_replication_group
without recreate resource: https://github.com/cloudposse/terraform-aws-elasticache-redis/issues/207
Thank you!