cloudposse / terraform-aws-elasticache-redis

Terraform module to provision an ElastiCache Redis Cluster
https://cloudposse.com/accelerate
Apache License 2.0
142 stars 246 forks source link

Add variables for parameter_group_name and parameter_group_create_on_destroy #189

Closed joshuabaird closed 7 months ago

joshuabaird commented 1 year ago

what

This PR adds two new variables:

why

This module doesn't currently support major version upgrades of Redis (eg, 6.x to. 7.x) because:

references

joshuabaird commented 1 year ago

Can we get this merged? @goruha

joshuabaird commented 1 year ago

/test all

aknysh commented 1 year ago

/test all

aknysh commented 1 year ago

@joshuabaird thanks for the PR Please see the errors https://github.com/cloudposse/actions/actions/runs/4376943663/jobs/7659766652

variables are not allowed in the lifecycle block. This is an annoying TF feature We usually get around that by creating two similar resources

resource "aws_elasticache_parameter_group" "default"
  count = module.this.enabled && var.parameter_group_create_before_destroy == false ? 1 : 0
resource "aws_elasticache_parameter_group" "create_before_destroy"
  count = module.this.enabled && var.parameter_group_create_before_destroy == true ? 1 : 0

and in the outputs.tf, select one or the other by using var.parameter_group_create_before_destroy ? xxx : yyy

please also run the following commands:

make init
make github/init
make readme

and commit the changes (GitHub token has been chamged in GH workflows, and the command make github/init will update all the GH workflows in the repo)

thank you

joshuabaird commented 1 year ago

@joshuabaird thanks for the PR Please see the errors https://github.com/cloudposse/actions/actions/runs/4376943663/jobs/7659766652

variables are not allowed in the lifecycle block. This is an annoying TF feature We usually get around that by creating two similar resources

I see that Terratest is failing, but this code does work. I'm using it. I'll take a look at your suggestion.

goruha commented 1 year ago

@joshuabaird Yes. We have so issue with our test infrastructure. I will rerun tests ASAP.

joshuabaird commented 1 year ago

Thinking about this more, I don't think we need to parameterize the lifecycle in the first place. I think this is easily solved by always setting create_before_destroy = true.