#pr-reviews (2023-12)

Pull Request Reviews for Cloud Posse Projects

2023-12-01

Kamil Grabowski avatar
Kamil Grabowski

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)

1
1
Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Jeremy White (Cloud Posse)

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Andriy Knysh (Cloud Posse)

Andriy Knysh (Cloud Posse) avatar
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

Kamil Grabowski avatar
Kamil Grabowski

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

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

before I asked you to update the description as well

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

since it uses the old logic for param group name

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

I’ll ask you to do the same again

Kamil Grabowski avatar
Kamil Grabowski

ok

Kamil Grabowski avatar
Kamil Grabowski

@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.

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

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

Kamil Grabowski avatar
Kamil Grabowski

I see, but there is no additional logic here, also local.enabled is not used in the code

Kamil Grabowski avatar
Kamil Grabowski

there is module.this.enabled everywhere

Kamil Grabowski avatar
Kamil Grabowski

of course I can change it, n/p, just wonder

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

not used? Then it’s not good Should be fixed

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

all our modules use local.enabled for the reasons explained above

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

if it’s defined and not used anywhere in the code, then it’s a declared and unused variable, which is a “code smell”

1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

I see what you are saying, the code has a bunch of count = module.this.enabled ? 1 : 0

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

can I ask you to fix that as well? (search for module.this.enabled everywhere and replace it with locl.enabled)

Kamil Grabowski avatar
Kamil Grabowski

sure

Kamil Grabowski avatar
Kamil Grabowski

I can replace module.this.enabled to local.enabled

Kamil Grabowski avatar
Kamil Grabowski

Done

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

thanks

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

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

1
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

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 -

Kamil Grabowski avatar
Kamil Grabowski

good point! I was testing with redis7. Let me fix it

Kamil Grabowski avatar
Kamil Grabowski

fixed

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

much better now thanks - approved and merged

1
Kamil Grabowski avatar
Kamil Grabowski

Thank you for your help!

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

Thanks @Andriy Knysh (Cloud Posse) !!

2023-12-21

Kamil Grabowski avatar
Kamil Grabowski

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

1
    keyboard_arrow_up