-
Notifications
You must be signed in to change notification settings - Fork 127
Conversation
I could not find a way to support 0.11.x and 0.12.x at the same time, so this goes all-in on 0.12.x features and cleans up a lot of code in the process. terraform-docs does not support 0.12.x yet so I've removed the Makefile and manually wrote the README. I have tried all tests in the test directory and they all worked.
archive.tf
Outdated
@@ -12,7 +6,7 @@ data "external" "archive" { | |||
query = { | |||
build_command = "${var.build_command}" |
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.
Do these all still need interpolation in 0.12?
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.
Thanks for reviewing this. I missed this file entirely as nothing broke in it. I'll clean it up.
@@ -7,27 +7,27 @@ data "aws_iam_policy_document" "assume_role" { | |||
|
|||
principals { | |||
type = "Service" | |||
identifiers = ["${slice(list("lambda.amazonaws.com", "edgelambda.amazonaws.com"), 0, var.lambda_at_edge ? 2 : 1)}"] | |||
identifiers = slice(list("lambda.amazonaws.com", "edgelambda.amazonaws.com"), 0, var.lambda_at_edge ? 2 : 1) |
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.
How far do you want to go to use new language features? This logic might become more straightforward, now that the ternary operator can return any type... https://www.terraform.io/docs/configuration/expressions.html#conditional-expressions
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.
Cool, that would be an improvement. I'll leave it as-is for now though, because I don't want to spend more time testing it.
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.
Ok, I'll turn the comment into an issue
|
||
name = "${var.function_name}-network" | ||
policy = "${data.aws_iam_policy_document.network.json}" | ||
policy = data.aws_iam_policy_document.network[0].json |
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.
Nice, I think I was afraid this representation would fail when count == 0
|
||
dead_letter_config { | ||
target_arn = "${var.dead_letter_config["target_arn"]}" | ||
dynamic "dead_letter_config" { |
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.
Woah. This is sweet.
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.
Gonna be a lot of folks needing to move resources in their state though, or let terraform resource cycle their lambdas. Still worth it I'd say.
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.
Yeah the dynamic blocks are good, although I still had to resort to a hack.
# this wouldn't work
dynamic "dead_letter_config" {
for_each = var.dead_letter_config == null ? [] : [var.dead_letter_config]
content {
target_arn = dead_letter_config.target_arn
}
}
# so i used a dummy value of 1 and accessed var.dead_letter_config from inside the dynamic block
dynamic "dead_letter_config" {
for_each = var.dead_letter_config == null ? [] : [1]
content {
target_arn = var.dead_letter_config.target_arn
}
}
And yeah this will change the resource location for anyone using dead letter queues or VPC, but I think it's worth it too. I started to implement S3 as a source and that was going to add another 4 permutations, I think. Definitely nicer having just the one resource.
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.
I figured out what I was doing wrong after reading the docs again.
dynamic "dead_letter_config" {
for_each = var.dead_letter_config == null ? [] : [var.dead_letter_config]
content {
target_arn = dead_letter_config.value.target_arn # <---- need to access .value to get the thing in from the list
}
}
👍 |
I could not find a way to support 0.11.x and 0.12.x at the same time, so this goes all-in on 0.12.x features and cleans up a lot of code in the process.
terraform-docs does not support 0.12.x yet so I've removed the Makefile and manually wrote the README.
I have tried all tests in the test directory and they all worked.