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

fix: default to AWS provided option and parameter groups when not creating nor providing #308

Conversation

bryantbiggs
Copy link
Member

Description

  • default to AWS provided option and parameter groups when not creating nor providing

Motivation and Context

Closes #303
Closes #305
Cloess #307

Breaking Changes

  • No

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
  • Validated using examples/complete-mysql and examples/complete-postgresql + added 2nd set of configs to both to prove this case here where a default group will be used by AWS when none is provided

@codezninja
Copy link
Contributor

I did a plan with my local project and confirmed that it works as I expected before this latest release.

@bryantbiggs
Copy link
Member Author

thanks @bondezbond !

main.tf Outdated
@@ -2,10 +2,10 @@ locals {
create_db_subnet_group = var.db_subnet_group_name == "" ? var.create_db_subnet_group : false
db_subnet_group_name = coalesce(var.db_subnet_group_name, module.db_subnet_group.this_db_subnet_group_id)
Copy link

@apjneeraj apjneeraj Mar 9, 2021

Choose a reason for hiding this comment

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

@bryantbiggs: It looks like similar change is required for db_subnet_group_name as function coalesce fails with same error due to 'module.db_subnet_group.this_db_subnet_group_id' parameter which does not has a value yet in case I try to create db_subnet_group.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, the subnet changes were coming down the pipe as well till this little snafu (master...bryantbiggs:fix/subnet-group) - I can put this back to the original though for the time being in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in c66fc9c

@sayertindall
Copy link

sayertindall commented Mar 9, 2021

I just cloned your fork and attempted to run the complete_postgresql example and it's still throwing the same error:
super new to Terraform, so this could totally be an error on my end.


  on ../../modules/db_parameter_group/main.tf line 12, in resource "aws_db_parameter_group" "this":
  12:   name_prefix = local.name_prefix```

@bryantbiggs
Copy link
Member Author

ya I'm not sure @sayerrrr what the error might be, I spun up both the complete-mysql and complete-postgresql as they are defined in this PR and no issue

@sayertindall
Copy link

@bryantbiggs interesting, let me see what i can come up with - appreciate your time!

@siozosdr
Copy link

siozosdr commented Mar 10, 2021

Will this fix also include the use-case of using the module to setup an RDS replica?

The error I'm currently getting is related to the mentioned issues #303 #305 #307:

Error: Error in function call

  on .terraform/modules/dis_replica/main.tf line 3, in locals:
   3:   db_subnet_group_name   = coalesce(var.db_subnet_group_name, module.db_subnet_group.this_db_subnet_group_id)
    |----------------
    | module.db_subnet_group.this_db_subnet_group_id is ""
    | var.db_subnet_group_name is ""

Call to function "coalesce" failed: no non-null, non-empty-string arguments.


Error: Error in function call

  on .terraform/modules/dis_replica/main.tf line 5, in locals:
   5:   parameter_group_name_id = coalesce(var.parameter_group_name, module.db_parameter_group.this_db_parameter_group_id)
    |----------------
    | module.db_parameter_group.this_db_parameter_group_id is ""
    | var.parameter_group_name is ""

Call to function "coalesce" failed: no non-null, non-empty-string arguments.


Error: first character of parameter group "name_prefix" must be a letter

  on .terraform/modules/dis_primary/modules/db_parameter_group/main.tf line 12, in resource "aws_db_parameter_group" "this":
  12:   name_prefix = local.name_prefix

I'm setting up a replica using the following options in my module:

module "dis_replica" {
  source  = "terraform-aws-modules/rds/aws"
  version = "2.25.0"

  identifier = "${local.dis_master_db_identifier}-replica1"

  apply_immediately = true

  auto_minor_version_upgrade = false

  # Source database. For cross-region use this_db_instance_arn
  replicate_source_db = module.dis_primary.this_db_instance_id

  engine                = local.dis_engine
  engine_version        = local.dis_engine_version
  instance_class        = local.dis_instance_class
  allocated_storage     = local.dis_allocated_storage
  max_allocated_storage = local.dis_max_allocated_storage
  storage_type          = local.dis_storage_type
  storage_encrypted     = local.dis_storage_encrypted
  port                  = local.dis_port

  # Username and password must not be set for replicas
  username = ""
  password = ""

  vpc_security_group_ids = [module.vpc.default_security_group_id, aws_security_group.dis_rds.id]

  maintenance_window = local.dis_maintenance_window
  backup_window      = local.dis_backup_window

  performance_insights_enabled          = local.dis_performance_insights_enabled
  performance_insights_retention_period = local.dis_performance_insights_retention_period

  deletion_protection = true
  # disable backups to create DB faster
  backup_retention_period = 0

  # Not allowed to specify a subnet group for replicas in the same region
  create_db_subnet_group = false

  create_db_option_group    = false
  create_db_parameter_group = false
}

@bryantbiggs
Copy link
Member Author

@siozosdr yes

@antonbabenko
Copy link
Member

@bryantbiggs Please let me know when you think it is ready to be merged.

@bryantbiggs
Copy link
Member Author

we should be all set @antonbabenko - did one last check using complete-mysql and everything still looks good :shipit:
image

@antonbabenko antonbabenko merged commit 8b8f9b7 into terraform-aws-modules:master Mar 10, 2021
@antonbabenko
Copy link
Member

Let's see! 🤞

v2.26.0 has been just released.

@bryantbiggs bryantbiggs deleted the fix/option-parameter-group-defaults branch March 10, 2021 13:14
@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 Nov 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants