#pr-reviews (2023-12)

@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

it does not like the dot in eg-test-redis-test-62525-redis6.x

https://github.com/cloudposse/terraform-aws-elasticache-redis/blob/main/examples/complete/fixtures.us-east-2.tfvars#L18

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

much better now thanks - approved and merged

Thank you for your help!

Thanks @Andriy Knysh (Cloud Posse) !!

done! https://github.com/cloudposse/terraform-aws-elasticache-redis/releases/tag/1.2.0

Thank you!