Skip to content
This repository was archived by the owner on Jan 31, 2021. It is now read-only.

Added ACM #3

Merged
merged 9 commits into from
Mar 18, 2019
Merged

Added ACM #3

merged 9 commits into from
Mar 18, 2019

Conversation

goruha
Copy link
Contributor

@goruha goruha commented Mar 11, 2019

What

  • Added ACM

Why

  • To support alb

@goruha goruha requested a review from tamsky March 11, 2019 19:01
tamsky
tamsky previously approved these changes Mar 11, 2019
Copy link

@tamsky tamsky left a comment

Choose a reason for hiding this comment

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

LGTM with removal of development module ... { source ="../.." changes.

@@ -28,7 +26,7 @@ module "subnets" {
}

module "codefresh_backing_services" {
source = "git::https://github.com/cloudposse/terraform-aws-codefresh-backing-services.git?ref=0.1.0"
Copy link

Choose a reason for hiding this comment

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

restore to original?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example easy to run based on the local directory.

Copy link

@tamsky tamsky left a comment

Choose a reason for hiding this comment

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

one more change to address

acm.tf Outdated
}

module "acm_request_certificate" {
source = "git::https://github.com/cloudposse/terraform-aws-acm-request-certificate.git?ref=add/enabled-var"
Copy link

Choose a reason for hiding this comment

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

ref=add/enabled-var => ref=tags/<release> ?

acm.tf Outdated

variable "acm_zone_name" {
type = "string"
default = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

it probably needs to be a required param

acm.tf Outdated

output "acm_id" {
value = "${module.acm_request_certificate.id}"
description = "The ARN of the certificate"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = "The ARN of the certificate"
description = "The ID of the certificate"

Copy link
Contributor

@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.

see comments

@goruha goruha requested review from Nuru and osterman March 18, 2019 00:45
@goruha goruha merged commit 8a3f617 into master Mar 18, 2019
@goruha goruha deleted the added-acm branch March 18, 2019 01:03
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants