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

Added support for same region replicas. #3

Conversation

jstojanovski-scwx
Copy link

Hi there... This fixes the fault I see with #2

Be gentle- It's my first PR for a terraform module. :)

@aknysh aknysh self-requested a review March 14, 2019 03:17
@aknysh
Copy link
Member

aknysh commented Mar 14, 2019

@jstojanovski-scwx thanks!
please run terraform fmt on all files, CI/CD is failing

@@ -21,7 +21,7 @@ module "final_snapshot_label" {
}

resource "aws_kms_key" "default" {
count = "${local.enabled && length(var.kms_key_id) == 0 ? 1 : 0}"
count = "${local.enabled && length(var.kms_key_id) == 0 && !var.same_region ? 1 : 0}"
Copy link
Member

@aknysh aknysh Mar 14, 2019

Choose a reason for hiding this comment

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

Suggested change
count = "${local.enabled && length(var.kms_key_id) == 0 && !var.same_region ? 1 : 0}"
count = "${local.enabled && length(var.kms_key_id) == 0 ? 1 : 0}"

I don't think we need to control the KMS key by var.same_region because if var.same_region = true, you always have to specify var.kms_key_id because of this expression

kms_key_id                = "${length(var.kms_key_id) > 0 ? var.kms_key_id : join("", aws_kms_key.default.*.arn)}"

but maybe a user needs to create a new key in the same region as the master instance (which let's say was created w/o a key)

@@ -63,7 +63,7 @@ resource "aws_db_instance" "default" {
}

resource "aws_db_subnet_group" "default" {
count = "${local.enabled ? 1 : 0}"
count = "${local.enabled && !var.same_region ? 1 : 0}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
count = "${local.enabled && !var.same_region ? 1 : 0}"
count = "${local.enabled && var.same_region == "false" ? 1 : 0}"

Since var.same_region is a string, not boolean

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @jstojanovski-scwx
Looks good, just a few comments.
And after you address the comments, please run terraform fmt to format all TF files, and then rebuild README by executing these command:

make init
make readme/deps
make readme

This will add the new variable to README.md

@jwhitcraft
Copy link
Contributor

Any update on getting the suggestions fixed @jstojanovski-scwx ?

If you don't have time, I can make the changes and push them to your branch.

@jstojanovski-scwx
Copy link
Author

@jwhitcraft Apologies... We stopped using this module, which meant it's fix priority dropped (for me). As a result, I haven't had the time to swing back around to fix it... If you have the cycles, please feel free. Otherwise, I'll get to it (just not for a while yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants