#pr-reviews (2023-12)
Pull Request Reviews for Cloud Posse Projects
2023-12-01
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
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)
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Jeremy White (Cloud Posse)
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
@Andriy Knysh (Cloud Posse)
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
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](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
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](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
before I asked you to update the description
as well
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
since it uses the old logic for param group name
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
I’ll ask you to do the same again
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
ok
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
@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](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
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](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
I see, but there is no additional logic here, also local.enabled
is not used in the code
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
there is module.this.enabled
everywhere
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
of course I can change it, n/p, just wonder
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
not used? Then it’s not good Should be fixed
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
all our modules use local.enabled
for the reasons explained above
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
if it’s defined and not used anywhere in the code, then it’s a declared and unused variable, which is a “code smell”
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
I see what you are saying, the code has a bunch of count = module.this.enabled ? 1 : 0
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
can I ask you to fix that as well? (search for module.this.enabled
everywhere and replace it with locl.enabled
)
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
sure
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
I can replace module.this.enabled
to local.enabled
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
Done
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
thanks
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
please see this error https://github.com/cloudposse/actions/actions/runs/7062118033/job/19225231599
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
family = "redis6.x"
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
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](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
good point! I was testing with redis7
. Let me fix it
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
fixed
![Andriy Knysh (Cloud Posse) avatar](https://avatars.slack-edge.com/2018-06-13/382332470551_54ed1a5d986e2068fd9c_72.jpg)
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
Thank you for your help!
![Erik Osterman (Cloud Posse) avatar](https://secure.gravatar.com/avatar/88c480d4f73b813904e00a5695a454cb.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0023-72.png)
Thanks @Andriy Knysh (Cloud Posse) !!
2023-12-21
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
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
![Dan Miller (Cloud Posse) avatar](https://avatars.slack-edge.com/2021-08-12/2389147782305_5729c9d69c393852d209_72.jpg)
![Kamil Grabowski avatar](https://secure.gravatar.com/avatar/fdd8663c691bc122156512f23c56cd38.jpg?s=72&d=https%3A%2F%2Fa.slack-edge.com%2Fdf10d%2Fimg%2Favatars%2Fava_0015-72.png)
Thank you!