feat: allow the creation of cross-region rds replicas #346
Conversation
|
Hi! How about changing the variable name from single-purpose The reason for this change is that there can be other situations where it makes sense to disable the creation of db_subnet_group in the future (if not already). |
|
Hi @antonbabenko thanks for your quick reply. I'm editing my comment cause i know understood what you mean :) So yes i can do it as you say, but i do think if we re use |
There was a problem hiding this comment.
I'd like to ask what @bryantbiggs thinks about the name for cross_region_replica variable and this PR?
main.tf
Outdated
| @@ -1,6 +1,6 @@ | |||
| locals { | |||
| master_password = var.create_db_instance && var.create_random_password ? random_password.master_password[0].result : var.password | |||
| db_subnet_group_name = var.replicate_source_db != null ? null : coalesce(var.db_subnet_group_name, module.db_subnet_group.db_subnet_group_id, var.identifier) | |||
| db_subnet_group_name = var.cross_region_replica != true && var.replicate_source_db != null ? null : coalesce(var.db_subnet_group_name, module.db_subnet_group.db_subnet_group_id, var.identifier) | |||
There was a problem hiding this comment.
@bryantbiggs local.db_subnet_group_name does not seem to be used in module db_subnet_group on line 24. Is it correct?
There was a problem hiding this comment.
correct - we only create the subnet group when create_db_subnet_group = true and the name is either db_subnet_group_name or identifier.
then, based on the logic on line 3 - we provide the subnet group name to the instance itself
Line 89 in e2636f9
There was a problem hiding this comment.
Yeah the issue is that if want to create a read only replica, db_subnet_group_name defaults to null, which is necessary for same region replicas but it is not correct for cross-region (different vpc, different subnets etc etc). I proposed the use of a different variable so that we can override that behaviour, and at the same time allow the use of an existing subnet group or the creation of a new one with the modukle, preserving the functionality of the module itself as is. Thank you :)
There was a problem hiding this comment.
@bryantbiggs WDYT? yes/no? :)
There was a problem hiding this comment.
I think this is ok - maybe we could change var.cross_region_replica != true && ... to !var.cross_region_replica && ...
we could also update the replica examples to add an instance in another region to ensure this works (and continues to work as intended with any future changes)
There was a problem hiding this comment.
I'm willing to do that for you if that is ok :)
|
¡Hola todos! Great to see so many approvals from Barcelona 🇪🇸 but let's hear if Bryant approves this, too :) |
antonbabenko
changed the title
|
Vamos! :) v3.3.0 has been just released. |
|
Thanks Anton for fixing it all!!!!! a pleasure. have a great day
…On Wed, 7 Jul 2021 at 16:32, Anton Babenko ***@***.***> wrote:
Vamos! :)
v3.3.0
<https://github.com/terraform-aws-modules/terraform-aws-rds/releases/v3.3.0>
has been just released.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#346 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQRECVSMVGXUS5FSZMHFGLTTWRQRRANCNFSM47OC6VAA>
.
|
|
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. |
Description
The changes adds a boolean called cross_region_replica.
Motivation and Context
In order to create a cross-region replica, we should specify a target subnet group, which is not needed for a same-region replica, as your module already supports with no issues. The idea is to set the cross_region_replica variable to true to allow the creation of the needed subnet group with minor changes to the source code
Breaking Changes
none
How Has This Been Tested?
i've tested this on both a same region and cross region replica.