-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
bugfix deprecate deployment_principal_arns
(#257)
#258
Conversation
/test all |
deprecated.tf
Outdated
@@ -7,6 +7,8 @@ locals { | |||
cloudfront_access_log_include_cookies = var.log_include_cookies == null ? var.cloudfront_access_log_include_cookies : var.log_include_cookies | |||
cloudfront_access_log_prefix = var.log_prefix == null ? var.cloudfront_access_log_prefix : var.log_prefix | |||
|
|||
deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : { for arn, path_prefix in var.deployment_principal_arns : arn => { "arn" : arn, "path_prefix" : path_prefix } } |
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.
This logic returns the following error in our tests
Error: Inconsistent conditional result types
on ../../deprecated.tf line 10, in locals:
10: deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : { for arn, path_prefix in var.deployment_principal_arns : arn => { "arn" : arn, "path_prefix" : path_prefix } }
Could you format this to make the logic easier to read? Id also put the second portion of the ternary in a separate local.
deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : { for arn, path_prefix in var.deployment_principal_arns : arn => { "arn" : arn, "path_prefix" : path_prefix } } | |
deployment_principal_arns = { | |
for arn, path_prefix in var.deployment_principal_arns : | |
arn => { "arn" : arn, "path_prefix" : path_prefix } | |
} | |
deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : local.deployment_principal_arns |
Id also try to run the examples/complete module locally to make sure it works as expected. It will be faster iteration.
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.
sorry for the delay here.. @nitrocode I've pushed a patch that fixes the readability. I'll report back on running the examples/complete
module.
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.
..hmm we don't use route 53, so I don't have an R53 zones setup. Is there another way to run some tests?
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.
@abeluck I re-ran the Terratest here and the result is the same as above
│ Error: Inconsistent conditional result types
│
│ on ../../deprecated.tf line 17, in locals:
│ 17: deployment_principals = var.deployment_principal_arns == null ? var.deployment_principals : local.deployment_principals_from_deprecated_deployment_principal_arns
│ ├────────────────
│ │ local.deployment_principals_from_deprecated_deployment_principal_arns is object with 2 attributes
│ │ var.deployment_principal_arns is map of list of string with 2 elements
│ │ var.deployment_principals is empty map of object
│
│ The true and false result expressions must have consistent types. The
│ 'true' value is map of object, but the 'false' value is object.
╵}
I believe the issues is because the example expects the map value to be a list of strings, whereas your variable is expecting a single prefix per arn.
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 @joe-niland It was an easy fix in the var definition.
I pared down the complete test to be able to run it locally. It's working now both with the deprecated var.deployment_principal_arns
and new var.deployment_principals
.
I've updated the documentation in a few more places, and rebased the branch so the PR can be easily squash merged.
We could also change the complete example to use the new var.deployment_principals
, what do you think?
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 @abeluck, that sounds good.
Yes, I think it makes sense to update the example.
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.
@abeluck sorry for the delay - all looks good, but will you be updating the examples?
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.
@abeluck see comment above re: examples :)
/terratest |
This patch refactors the passing of deployment principals, such that it uses static/known map keys. This allows this module to be applied in the same terraform run that the deployment principal (e.g., an iam user) is applied. Hashicorp recommends storing only known values in map keys, and leaving all dynamic/unknown values in the map values ([source0](https://developer.hashicorp.com/terraform/language/meta-arguments/for_each#limitations-on-values-used-in-for_each), [source1](hashicorp/terraform#30838 (comment))).
/terratest edit: ah I suppose I don't have perms to trigger this. |
Hi @abeluck can you please run the following and commit the result?
|
Done! |
Looking forward to this getting merged! |
/terratest |
@joe-niland let me know if there is anything else I can do! |
@joe-niland can you follow up on this one? |
@abeluck if you have a minute to update the main example to use |
/terratest |
This pull request now has conflicts. Could you fix it @abeluck? 🙏 |
This PR has been closed due to inactivity and merge conflicts. |
Thanks @abeluck for creating this pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. While you wait, make sure to review our contributor guidelines. Tip Need help or want to ask for a PR review to be expedited?Join us on Slack in the |
what
deployment_principal_arns
withdeployment_principals
why
(source0, source1).
references