Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

abeluck
Copy link

@abeluck abeluck commented Feb 14, 2023

what

  • This patch refactors the passing of deployment principals, such that it uses static/known map keys.
  • It replaces deployment_principal_arns with deployment_principals

why

  • This allows this module to be applied at the same time as the deployment principal (e.g., an iam user) is is deployed.
  • Hashicorp recommends storing only known values in map keys, and leaving all dynamic/unknown values in the map values
    (source0, source1).

references

@abeluck abeluck requested review from a team as code owners February 14, 2023 15:50
@abeluck abeluck requested review from jamengual and woz5999 and removed request for a team February 14, 2023 15:50
@goruha
Copy link
Member

goruha commented Feb 16, 2023

/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 } }
Copy link
Member

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.

Suggested change
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.

Copy link
Author

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.

Copy link
Author

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?

Copy link
Sponsor Member

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.

Copy link
Author

@abeluck abeluck Aug 11, 2023

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?

Copy link
Sponsor Member

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.

Copy link
Sponsor Member

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?

Copy link
Sponsor Member

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 :)

@joe-niland
Copy link
Sponsor Member

/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))).
@abeluck
Copy link
Author

abeluck commented Aug 11, 2023

/terratest

edit: ah I suppose I don't have perms to trigger this.

@joe-niland
Copy link
Sponsor Member

Hi @abeluck can you please run the following and commit the result?
Just need to update README.md

make init
make readme

@abeluck
Copy link
Author

abeluck commented Sep 7, 2023

can you please run the following and commit the result?

Done!

@deiga
Copy link

deiga commented Nov 6, 2023

Looking forward to this getting merged!

@joe-niland
Copy link
Sponsor Member

/terratest

@abeluck
Copy link
Author

abeluck commented Nov 9, 2023

@joe-niland let me know if there is anything else I can do!

@hans-d
Copy link

hans-d commented Mar 2, 2024

@joe-niland can you follow up on this one?

@joe-niland
Copy link
Sponsor Member

@abeluck if you have a minute to update the main example to use deployment_principals then I think we're good to go. Sorry for the delay.

@hans-d
Copy link

hans-d commented Mar 4, 2024

/terratest

Copy link

mergify bot commented Mar 9, 2024

This pull request now has conflicts. Could you fix it @abeluck? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Mar 9, 2024
Copy link

mergify bot commented Mar 9, 2024

This PR has been closed due to inactivity and merge conflicts.
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot closed this Mar 9, 2024
Copy link

mergify bot commented Mar 9, 2024

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 #pr-reviews channel.

@mergify mergify bot added triage Needs triage and removed conflict This PR has conflicts triage Needs triage labels Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot create deployment principal and the CDN in the same terraform run
6 participants