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

Replace try() with lookup() where possible #227

Closed
matt-FFFFFF opened this issue Nov 26, 2021 · 9 comments
Closed

Replace try() with lookup() where possible #227

matt-FFFFFF opened this issue Nov 26, 2021 · 9 comments
Labels
enhancement New feature or request
Milestone

Comments

@matt-FFFFFF
Copy link
Member

matt-FFFFFF commented Nov 26, 2021

Within the module we make regular use of the try() function to allow optional fields to be gracefully skipped when not present in the input variable.

A side-effect of this is a lot of noise in the verbose logging, but this is possibly not the most efficient function to use.

To reduce the impact of this and provide more logical handling of scenarios where the base object exists, but a specific key may or may not exist, the lookup() function would make more sense.

@matt-FFFFFF matt-FFFFFF added the enhancement New feature or request label Nov 26, 2021
@matt-FFFFFF matt-FFFFFF added this to the 1.2.0 milestone Nov 26, 2021
@krowlandson krowlandson changed the title Replace try() with lookup() where possible Replace try() with lookup() where possible Nov 26, 2021
@roberthstrand
Copy link

If no one is currently working on this, I could take this on. I don't want to just start doing stuff on my own if someone has already started to work on this 😊

@krowlandson
Copy link
Contributor

krowlandson commented Dec 5, 2021

@roberthstrand... No-one working on this yet from our side so if you'd like to contribute that would be awesome.

Just be aware that there are still a lot of legitimate cases where we will still need to use try() for cases where the input of a complex object is optional.

We could probably remove these completely as a stretch goal, but would require building up the objects in layers using tertiaries.

@krowlandson
Copy link
Contributor

@roberthstrand - just a heads up that I am partially covering this as part of an upcoming update. The files I will be resolving include the following:

  • main.tf
  • locals.tf
  • connectivity/locals.tf

@roberthstrand
Copy link

@roberthstrand - just a heads up that I am partially covering this as part of an upcoming update. The files I will be resolving include the following:

  • main.tf

  • locals.tf

  • connectivity/locals.tf

I went on Christmas vacation and forgot all about this. I will look over the next version and see what remains, thanks for letting me know.

@krowlandson
Copy link
Contributor

Upon further investigation and testing, care needs to be taken when using lookup() to avoid errors similar to the following:

╷
│ Error: Invalid function argument
│
│   on ..\..\..\main.tf line 49, in module "management_resources":
│   49:   custom_settings_by_resource_type             = lookup(local.management_resources_advanced, "custom_settings_by_resource_type", local.empty_map)
│     ├────────────────
│     │ local.empty_map is object with no attributes
│
│ Invalid value for "default" parameter: the default value must have the same type as the map elements.
╵

Workaround needed before this can be successfully implemented.

@krowlandson
Copy link
Contributor

Workaround for the above... if you want an object() instead of a map() in Terraform, create an object() and merge your input into it...

image

I could have done this with actual expected keys with default values, so merge overrides the values, but it felt like a more generic approach would avoid confusion and make the base object generic.

@jtracey93
Copy link
Collaborator

Trigger ADO Sync

krowlandson pushed a commit to krowlandson/terraform-azurerm-caf-enterprise-scale that referenced this issue Sep 12, 2022
krowlandson pushed a commit that referenced this issue Sep 13, 2022
* Add logic to map automation account to region

* Replace `try` with `lookup` as per #227
@krowlandson
Copy link
Contributor

Trigger ADO Sync

@krowlandson
Copy link
Contributor

Closing this issue as this is now considered a coding standard for this module and will be addressed over time as code is refactored.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants