support final_snapshot_identifier #3
Conversation
modules/db_instance/variables.tf
Outdated
| @@ -39,6 +39,10 @@ variable "port" { | |||
| description = "The port on which the DB accepts connections" | |||
| } | |||
|
|
|||
| variable "final_snapshot_identifier" { | |||
| description = "The name of your final DB snapshot when this DB instance is deleted." | |||
| } | |||
There was a problem hiding this comment.
There should be default = "", because this is an optional argument.
There was a problem hiding this comment.
Since skip_final_snapshot defaults to false (https://github.com/terraform-aws-modules/terraform-aws-rds/blob/master/modules/db_instance/variables.tf#L108) And the AWS api wont allow you to delete an RDS instance without a final_snapshot_identifier set unless skip_final_snapshot = true, I'm not sure what is the best setting here. It seems to be that either skip_final_snapshot should be true by default, which I would consider a bad design decision as it could have unwanted effects. So my thought was to force the identification of final_snapshot_identifier instead.
There was a problem hiding this comment.
This resource module does not have to be smarter than Terraform & AWS API, so keeping it as default = "" is a good solution. Please fix and we merge this PR.
There was a problem hiding this comment.
👍 @antonbabenko should be fixed now.
variables.tf
Outdated
| @@ -19,6 +19,10 @@ variable "engine_version" { | |||
| description = "The engine version to use" | |||
| } | |||
|
|
|||
| variable "final_snapshot_identifier" { | |||
| description = "The name of your final DB snapshot when this DB instance is deleted." | |||
| } | |||
| @@ -23,39 +23,42 @@ module "db" { | |||
| source = "terraform-aws-modules/rds/aws" | |||
|
|
|||
| identifier = "demodb" | |||
|
|
|||
There was a problem hiding this comment.
Good catch with all these spaces.
…ules#3) * update examples with terraform-aws-kms module * add description
…ules#3) * update examples with terraform-aws-kms module * add description
|
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. |
In its current state the module will provision an RDS instance just fine, however, because skip_final_snapshot is set to false by default AWS wont allow the RDS instance to be deleted. The only way I found to solve this was to require the setting of final_snapshot_identifier as I wanted to be conservative and not default to skipping the final snapshot by default.