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

updating to allow conditional creation of the cdn and dns #31

Closed

Conversation

dannybrody
Copy link

Currently we require the ability to conditionally create the aws_cloudfront_distribution. Updated to allow the conditional creation of resource "aws_cloudfront_distribution" "default" together with module "dns". Also updated the outputs to work with both cases.

main.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
outputs.tf Outdated Show resolved Hide resolved
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 @dannybrody
please see comments.
Also, since you are at it anyway, you can add count = var.enabled == "true" ? 1 : 0 to all other resources (label, logs, dns) - var.enabled should enable/disable all resources in the module

@dannybrody
Copy link
Author

@aknysh sounds great, I'll update, test, and then update the PR. Thanks for responding so quickly!

…le to version 0.3.0 since that is the minimum version that has the 'enabled' variable
@dannybrody
Copy link
Author

@aknysh I went ahead and tried updated the other modules with count = var.enabled == "true" ? 1 : 0 however, it tried to destroy my resources. when i updated it to enabled = "${var.enabled ? true : false }" it worked as expected

@dannybrody
Copy link
Author

Any updates on this?

@dannybrody
Copy link
Author

updated due to running into the following plan error:
Error: module.application_cloudfront.aws_cloudfront_distribution.default: restrictions.0.geo_restriction.0.locations: should be a list

main.tf Outdated
@@ -1,5 +1,6 @@
module "origin_label" {
source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.3.7"
enabled = "${var.enabled ? true : false }"
Copy link
Member

@aknysh aknysh Jan 10, 2020

Choose a reason for hiding this comment

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

Suggested change
enabled = "${var.enabled ? true : false }"
enabled = "${var.enabled}"

main.tf Outdated
@@ -13,7 +14,8 @@ resource "aws_cloudfront_origin_access_identity" "default" {
}

module "logs" {
source = "git::https://github.com/cloudposse/terraform-aws-log-storage.git?ref=tags/0.2.2"
source = "git::https://github.com/cloudposse/terraform-aws-log-storage.git?ref=tags/0.3.0"
enabled = "${var.enabled ? true : false }"
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
enabled = "${var.enabled ? true : false }"
enabled = "${var.enabled}"

main.tf Outdated
@@ -28,6 +30,7 @@ module "logs" {

module "distribution_label" {
source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=tags/0.3.7"
enabled = "${var.enabled ? true : false }"
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
enabled = "${var.enabled ? true : false }"
enabled = "${var.enabled}"

main.tf Outdated
@@ -37,6 +40,7 @@ module "distribution_label" {
}

resource "aws_cloudfront_distribution" "default" {
count = "${var.enabled ? 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 = "${var.enabled ? 1 : 0 }"
count = "${var.enabled == "true" ? 1 : 0 }"

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 @dannybrody
Please see comments and run terraform fmt and rebuild README by executing:

make init
make readme/deps
make readme
``

@maximmi
Copy link
Contributor

maximmi commented Feb 25, 2020

/rebuild-readme

@maximmi
Copy link
Contributor

maximmi commented Feb 25, 2020

/terraform-fmt

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.

None yet

4 participants