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.

Sign up for GitHub

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 ability to define custom DB Subnet Group Name #372

Closed
wants to merge 4 commits into from
+14 −2
Closed

Add ability to define custom DB Subnet Group Name #372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fad1657
Add ability to define custom DB Subnet Group Name
stuzlogle Dec 10, 2019
8aa0c62
Fix custom db group name feature to be optional instead of static
stuzlogle Dec 10, 2019
7ef67fc
set default null for db_subnet_group_name so eval happens correctly
stuzlogle Dec 12, 2019
4d8becc
Add custom elasticache subnet_group name option
stuzlogle Dec 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ resource "aws_subnet" "database" {
resource "aws_db_subnet_group" "database" {
count = var.create_vpc && length(var.database_subnets) > 0 && var.create_database_subnet_group ? 1 : 0

name = lower(var.name)
name = var.database_subnet_group_name != null ? lower(var.database_subnet_group_name) : lower(var.name)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be able to get away with:

Suggested change
name = var.database_subnet_group_name != null ? lower(var.database_subnet_group_name) : lower(var.name)
name = coalesce(lower(var.database_subnet_group_name, var.name))

description = "Database subnet group for ${var.name}"
subnet_ids = aws_subnet.database.*.id

Expand Down Expand Up @@ -443,7 +443,7 @@ resource "aws_subnet" "elasticache" {
resource "aws_elasticache_subnet_group" "elasticache" {
count = var.create_vpc && length(var.elasticache_subnets) > 0 && var.create_elasticache_subnet_group ? 1 : 0

name = var.name
name = var.elasticache_subnet_group_name != null ? lower(var.elasticache_subnet_group_name) : lower(var.name)
Copy link
Contributor

@y-batsianouski y-batsianouski Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You added lower(var.name). It will break infrastructure of the current users of this module during upgrade to the new version.
Not sure you need add lower() here at all.

Copy link
Member

Choose a reason for hiding this comment

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

ya, not sure why some have lower() and some don't, but lets maintain status quo for now:

Suggested change
name = var.elasticache_subnet_group_name != null ? lower(var.elasticache_subnet_group_name) : lower(var.name)
name = coalesce(var.elasticache_subnet_group_name, var.name)

description = "ElastiCache subnet group for ${var.name}"
subnet_ids = aws_subnet.elasticache.*.id
}
Expand Down
12 changes: 12 additions & 0 deletions variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ variable "name" {
default = ""
}

variable "database_subnet_group_name" {
description = "Name to be used on DB Subnet Group resource as identifier"
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add rules for the DB subnet group name.
Must contain from 1 to 255 characters. Alphanumeric characters, spaces, hyphens, underscores, and periods are allowed.

Copy link
Member

Choose a reason for hiding this comment

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

agreed - I think this would be helpful for users to know if there is criteria that must be followed

type = string
default = null
}

variable "elasticache_subnet_group_name" {
description = "Name to be used on Elasticache Subnet Group resource as identifier"
type = string
default = null
}

variable "cidr" {
description = "The CIDR block for the VPC. Default value is a valid CIDR, but not acceptable by AWS and should be overridden"
type = string
Expand Down