-
Notifications
You must be signed in to change notification settings - Fork 95
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
fix: update PE spec to work with Azure Policy #537
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.
I am not fully versed with the context of this change. My only suggestion (and this may be a personal preference) is to shift complex logic to the left.
By that I mean:
- Do as much as you can with variables and
optional()
to shift logic there. - Do other complex logic in
locals
and output a map where relevant (or a boolean if it is just a resource exists or does not exist). For dynamic blocks it can also be a list. - Then the resource itself just accepts the relevant and validated
local
in for_each, count and and dynamic blocks.
For me this makes the code easier to read as mixing complex logic in resource
and data source
declarations can be difficult to read. It also make debugging a bit easier sometimes as you just output the local to see if it looks right. It can also help to eliminate repetitive code, such as in the MI example, which could be a single dynamic block following this approach.
Thanks Jared, I'll look to include more locals in the identity examples. Appreciate the review. |
Ref: Azure/terraform-azurerm-avm-res-keyvault-vault#32
Also incorporates minor changes to tags, rbac, mi to set nullable to false