Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add variables for parameter_group_name and parameter_group_create_on_destroy #189

Closed

Conversation

joshuabaird
Copy link

what

This PR adds two new variables:

  • parameter_group_name
  • parameter_group_create_before_destroy

why

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

  • Parameter groups are major-version specific, so when a user changes var.family from redis6 to redis7, Terraform needs to create a new parameter group. Without create_before_destroy, Terraform tries to first destroy the old Param group which fails because it's currently in use
  • Parameter groups must have unique names across families. When Terraform tries to create a new param group for redis7, it fails because it tries to do so using the same name as the old param group.

references

@joshuabaird joshuabaird requested review from a team as code owners January 10, 2023 17:27
@joshuabaird joshuabaird requested review from r351574nc3 and srhopkins and removed request for a team January 10, 2023 17:27
@joshuabaird
Copy link
Author

joshuabaird commented Mar 9, 2023

Can we get this merged? @goruha

@joshuabaird
Copy link
Author

/test all

1 similar comment
@aknysh
Copy link
Member

aknysh commented Mar 9, 2023

/test all

@aknysh
Copy link
Member

aknysh commented Mar 9, 2023

@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

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please see comments

@joshuabaird
Copy link
Author

@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
Copy link
Member

goruha commented Mar 14, 2023

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

@joshuabaird joshuabaird force-pushed the chore/param-group-name branch from 6f50103 to 4888371 Compare April 7, 2023 19:15
@joshuabaird
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This PR has gone stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_elasticache_parameter_group should have create_before_destroy set to true
4 participants