-
Notifications
You must be signed in to change notification settings - Fork 13
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
First pass at updating for tf12 #9
Conversation
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.
Generally looks good to me. Had a question about that workaround local-exec thing though.
|
||
policy = "${data.aws_iam_policy_document.logs_policy_doc.json}" | ||
policy = data.aws_iam_policy_document.logs_policy_doc.json | ||
} | ||
|
||
# This code verifies that the count of policy ARNs matches the actual |
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 we still need this workaround? I'm not sure I understand the particular limitation that Terraform has/had here and if that got fixt in TF12?
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've been thinking about potentially doing this in 2 phases
- to make our modules syntactically work with 0.12
- refactor the modules to more closely align with the 0.12 feature set.
That being said I'm open to other opinions.
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 think this was the Terraform issue that was forcing this workaround: hashicorp/terraform#14677. This is supposedly fixed in 0.12 from the sound of it. I tend to agree with @dynamike that a two-stage process works pretty well. I also wanted to try and get this sorted out ASAP under the assumption we can make it prettier later.
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.
in which case, I'll approve.
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.
YASSSSS UPGRADE
I went through and made the necessary changes to get this working in TF12; I was able to successfully create a lambda job in the sandbox account using this code.