-
-
Notifications
You must be signed in to change notification settings - Fork 567
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
Conditional creation of cluster and related resources #7
Conditional creation of cluster and related resources #7
Conversation
@antonbabenko can you please review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments (mostly newlines after count
)
main.tf
Outdated
@@ -83,15 +84,13 @@ data "aws_iam_policy_document" "monitoring_rds_assume_role" { | |||
} | |||
|
|||
resource "aws_iam_role" "rds_enhanced_monitoring" { | |||
count = "${var.monitoring_interval > 0 ? 1 : 0}" | |||
|
|||
count = "${var.create_resources ? var.monitoring_interval > 0 ? 1 : 0 : 0}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less comparisons is nicer, I believe:
count = "${var.create_resources ? var.monitoring_interval > 0 ? 1 : 0 : 0}" | |
count = "${var.create_resources && var.monitoring_interval > 0 ? 1 : 0}" |
main.tf
Outdated
@@ -129,6 +128,7 @@ resource "aws_appautoscaling_policy" "autoscaling_read_replica_count" { | |||
} | |||
|
|||
resource "aws_security_group" "this" { | |||
count = "${var.create_resources}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep empty lines after count
and always have it as the first one because this is meta-argument.
main.tf
Outdated
@@ -137,8 +137,7 @@ resource "aws_security_group" "this" { | |||
} | |||
|
|||
resource "aws_security_group_rule" "default_ingress" { | |||
count = "${length(var.allowed_security_groups)}" | |||
|
|||
count = "${var.create_resources ? length(var.allowed_security_groups) : 0}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. New line is missing.
Hi there! @max-rocket-internet, I'm wondering why this feature wasn't delivered? |
Hi @igat64, sorry I can't remember exactly. Perhaps because the review process was going too slowly. Here's our copy of the module, the original version of the module here: https://github.com/deliveryhero/tf-aws-rds-aurora |
Ok, thank you @max-rocket-internet ! |
…eam-master [HARRI-136652]: Sync the upstream plus the custom changes
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. |
Adding
create_resources
to allow declaring the module without creating any resources. This is mentioned in #2