Fix #4 & enhanced RedShift functionality #5
Conversation
Fixes the issue “Existing RedShift subnet group not honored” (#4) where subnet groups / parameter groups were being incorrectly created.
- Updated the list of instance types - Added `final_snapshot_identifier` functionality - Adding `logging` block functionality
sc250024
changed the title
main.tf
Outdated
| @@ -55,7 +65,7 @@ resource "aws_redshift_parameter_group" "this" { | |||
| } | |||
|
|
|||
| resource "aws_redshift_subnet_group" "this" { | |||
| count = "${local.enable_create_redshift_subnet_group}" | |||
| count = "${var.redshift_subnet_group_name != "" ? 0 : 1}" | |||
There was a problem hiding this comment.
It should be == instead of !=
There was a problem hiding this comment.
Come to think of it, should we change the locals to be similar to the Terraform RDS module? https://github.com/terraform-aws-modules/terraform-aws-rds/blob/master/main.tf#L1-L6
There was a problem hiding this comment.
Yes, using locals is much better.
There was a problem hiding this comment.
The way it was implemented before, using the blocks:
enable_create_redshift_subnet_group = "${var.redshift_subnet_group_name == "" ? 0 : 1}"
enable_create_redshift_parameter_group = "${var.parameter_group_name == "" ? 0 : 1}"Was actually was caused the problem. If you look at my logs in #4, for some reason, using enable_create_redshift_ the way it was written resulted in subnet groups being created even when a user specified an already existing subnet group.
In the RDS module, the difference is that the code block:
enable_create_db_subnet_group = "${var.db_subnet_group_name == "" ? var.create_db_subnet_group : 0}"
enable_create_db_parameter_group = "${var.parameter_group_name == "" ? var.create_db_parameter_group : 0}"Has explicit create_db_* variables. Do we want equivalent create_redshift_parameter_group and create_redshift_subnet_group variables in this module?
main.tf
Outdated
| @@ -37,13 +38,22 @@ resource "aws_redshift_cluster" "this" { | |||
|
|
|||
| tags = "${var.tags}" | |||
|
|
|||
| # Enhanced VPC routing | |||
| enhanced_vpc_routing = "${var.enhanced_vpc_routing}" | |||
|
|
|||
| lifecycle { | |||
There was a problem hiding this comment.
Move tags and lifecycle arguments at the end as they were before.
|
@antonbabenko My latest commit be8eeab restores most of the functionality for the locals
Hopefully this resolves things, and can be merged now. |
|
Took me some time to think about it and understand it. Good job! Merging now. |
|
v1.1.0 has been released. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Fixes issue #4, and adds other functionality to the RedShift module.
Description
parameter_group_nameis not specified since RedShift will create a default parameter group for you. However, ifredshift_subnet_group_nameis not specified, then the resourceredshift.aws_redshift_subnet_group.thisshould be created.cluster_typebased on the number of nodes the user specifies => 0b0dc7d and fca695dallow_version_upgrade=> fc6c31efinal_snapshot_identifier=> 7c86caaMotivation and Context
Initially I was attempting to solve #4, and noticed there was some other functionality I would like included in the module as well.
How Has This Been Tested?
Tested locally with AWS VPC creation of RedShift resources.
Types of changes
Checklist: