-
-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fix #4 & enhanced RedShift functionality #5
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor comments here and there.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be ==
instead of !=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using locals
is much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move tags
and lifecycle
arguments at the end as they were before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@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_name
is not specified since RedShift will create a default parameter group for you. However, ifredshift_subnet_group_name
is not specified, then the resourceredshift.aws_redshift_subnet_group.this
should be created.cluster_type
based 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: