Use ternary instead of && for boolean logic #84
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Logical binary operators don't short-circuit: hashicorp/terraform#24128
Summary
In terraform, logical binary operators don't short-circuit: hashicorp/terraform#24128
So while the code here looks like each branch of the conditional check is checking to see whether the second check is necessary, terraform (or more strictly/accurately, HCL) is always evaluating all of the branches of the boolean expression.
The problem with that is that, for example, if you don't need to check the
var.bucket_sse_key_arn
becausevar.use_existing_cloudtrail
is true, but yourvar.bucket_sse_key_arn
that you passed in is referencing another resource, this will create a terraform error because it can't determine the ultimate value of thiscreate_kms_key
local which is used to determine thecount
of the kms key resource.And so, we get this error:
By using the ternary operator instead of the && operator, we effectively make it "short circuit" and not evaluate the second part of the boolean expression, while retaining the same logic as before. This allows combinations of variables that would disable the kms key from being created to work without the error above.
How did you test this change?
We used this forked code in our production terraform module where we were trying to upgrade to v2.x from v0.2, and this unblocked us from upgrading.
Issue
This is related to #81 but does not completely fix that issue. It does fix the case where you are using an existing cloudtrail resource, i.e.
var.use_existing_cloudtrail = true
, because then it doesn't need to check thebucket_sse_key_arn
variable.A further fix would be required to enable passing a custom bucket key and using an existing cloudtrail. Probably we need to add a
var.use_existing_bucket_sse_key
variable.