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

Conversation

stuzlogle
Copy link

Description

TF Will destroy and recreate any aws_db_subnet_group resource which has a name change. Allowing the module to specify a name allows users to import existing resources into the vpc managed by the module.

@y-batsianouski
Copy link
Contributor

y-batsianouski commented Sep 23, 2020

Upvote for this PR! It's really useful when you need to import existed resources into this module and you have over 50 RDS instances in existed db subnet group on production :)

@stuzlogle I think you need update this PR and add custom name for redshift too so this PR will cover all subnet groups for all services.

@@ -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

@@ -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

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.

@bryantbiggs bryantbiggs self-requested a review February 23, 2021 00:18
@bryantbiggs bryantbiggs self-assigned this Feb 23, 2021
@@ -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))

@@ -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
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)

@bryantbiggs
Copy link
Member

@stuzlogle left you some feedback - I know its been a minute since you opened the PR so let me know if you want us to carry it over the line

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants