#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!