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

fix(terraform): set null value as fallback for missing variables #7669

Merged

Conversation

albertodonato
Copy link
Contributor

Description

This adds a default null value for variables that don't get a value (either supplied or default).
It allows expressions using those variables to be evaluated, rather than just be set to null.
E.g

variable "v" {
  type = string
}

locals {
  l = ["foo", var.v]
  d = {"foo": var.v}
}

would produce l = ["foo" null] and d = {"foo": null} rather than just being both null.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@albertodonato albertodonato force-pushed the missing-variables-null-value branch from 1446748 to 0b6dcd4 Compare October 8, 2024 11:30
@simar7
Copy link
Member

simar7 commented Oct 10, 2024

@nikpivkin tests are red due to the rate limit issue, it lgtm, should we merge it?

@simar7
Copy link
Member

simar7 commented Oct 10, 2024

thanks for your contribution, @albertodonato

@nikpivkin
Copy link
Contributor

Hi @albertodonato ! I'm wondering what problem this solves. Do you have an example?

variable "v" {
  type = string
}

locals {
  l = ["foo", var.v]
  d = {"foo": var.v}
}

In the configuration above, local.l would equal [“foo”, cty.unknownType], a local.d would equal {“foo”: cty.unknownType}.

@albertodonato
Copy link
Contributor Author

@nikpivkin as mentioned above, in my test without this change, the locals would get set to null because the variable is unknown, whereas with this change you at least get the expected type (list, map) with null values.

@albertodonato
Copy link
Contributor Author

FWIW similar behavior happens with data references when the data attribute isn't defined, but that seems to take a different path in code

@nikpivkin
Copy link
Contributor

@simar7 I restarted the tests and they passed

@nikpivkin
Copy link
Contributor

locals would get set to null because the variable is unknown

The value does not become null if one of the elements is unknown. I have given an example above.

@albertodonato
Copy link
Contributor Author

locals would get set to null because the variable is unknown

The value does not become null if one of the elements is unknown. I have given an example above.

If you revert my change the added test fails on the s (which is null) and similarly l2 and d2 seem to be null.
Perhaps there's a way to make a more explicit check for the actual value in the tests? I didn't find one.

@albertodonato
Copy link
Contributor Author

@nikpivkin would it be possible to merge this, or is something needed on my side?

@nikpivkin
Copy link
Contributor

@simar7 can you take a look?

@simar7 simar7 added this pull request to the merge queue Nov 8, 2024
Merged via the queue into aquasecurity:main with commit 611558e Nov 8, 2024
12 checks passed
@aqua-bot aqua-bot mentioned this pull request Nov 8, 2024
@albertodonato albertodonato deleted the missing-variables-null-value branch November 8, 2024 15:40
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.

3 participants