-
-
Notifications
You must be signed in to change notification settings - Fork 980
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
do not pass null value variables #1367
base: main
Are you sure you want to change the base?
Conversation
Thanks for the contribution! I disagree with this:
As you may want to explicitly set variables to I think #1267 is the proper fix for this, as the intention of setting input to I think a better fix for what you want would be to introduce a helper function that filters out map keys with null values (similar to Note that you should be able to already do this today using
With a helper, that would look like:
|
Thank you for the reply, The workaround you mention would be helping me already, thanks for that. Additionally, I agree that the tfvars file would be able to handle 'null' value variables (unfortunately unsupported on TF_VAR_*) but it has the trade-off of provably write 'sensitive' information to disk (which may a blocker too for my requirements). The PR approach is to follow same Terraform behavior with variables, these are not included in any definition when null,. For example, in Terragrunt you cannot consume a Terraform output 'null' value, variable does not exist, you need to threat it as if not defined. Same rules apply on my implementation. Look at the below terraform example:
The output will exist depending if null or not:
If no value is provided, no output exist:
If make sense, I can rework |
Yes this is the reason the PR is blocked, and it won't be merged until we figure out a way to avoid this. Note that if we were focused on solving the
Yes I understand this approach. I wanted to highlight that there are use cases where it makes sense to not have a default but handle Here is an example: suppose you had two ways of getting the AMI: one is to get the AMI by ID, and the other is to get it by filters. You might define it as such in your module: variable "ami_id" {
type = string
}
variable "ami_version_tag" {
type = string
}
data "aws_ami" "example" {
count = var.ami_version_tag != null && ami_id == null ? 1 : 0
executable_users = ["self"]
most_recent = true
name_regex = "^myami-\\d{3}"
owners = ["self"]
filter {
name = "name"
values = ["myami-version-${var.ami_version_tag}-*"]
}
# ...other filters intentionally omitted...
}
resource "aws_instance" "web" {
ami = var.ami_id != null && var.ami_version_tag == null ? var.ami_id : data.aws_ami.example[0].id
# ... other vars omitted for brevity ...
} We want both to be required so that the user gets a reasonable error if they don't set either of them, because an AMI is required. Yes you can easily workaround this by setting default to
We're generally weary of terragrunt feature flags on the CLI as it goes against the spirit of IaC. It would be better if the setting was captured in the config so that it is explicit, and you don't have to remember to pass in the flag all the time (or worse, when you have mixed requirements where some modules need the flag and others do not, and dealing with the flag during an This is why I suggested using a function that explicitly filters the nulls out. This would be captured in the config, and you have control over when it should be done and not, and it is locally scoped. The function is, IMO, also much more intuitive than a feature flag because it works on primitive data structures and thus does not worry about things like if the var is already defined as However, most of this (function over attribute feature flag) is opinionated so if you have a reason why the function approach does not work for you, happy to consider the attribute approach. But I do feel strongly that the CLI flag is not the right approach here. |
I know that it may be not what Terragrunt group would like to implement, but as per Terraform null value definition it means:
The important part: it will use the argument's default value if it has one, or raise an error if the argument is mandatory If the problem is readability of the error, potentially Terragrunt may print out a warning message when a variable is set to null. Thanks |
This is provably incomplete, because if you provide both inputs it will still bring a confusing error (or even an unexpected situation) if that is the case, this should be validated, for example, Forseti Security implements a validation step: in This example:
Or I have created my own validator for these situations where they are mutually exclusive:
|
The problem with literally using the terraform definition is that it doesn't work like that when you set the variable to For example, suppose you had a module as such:
And you invoked it with:
By your interpretation of the terraform defintion, it should output "hello world" for Terragrunt is just feeding variables to modules, so as long as |
To explain a different way, the mental model of terragrunt input passing is to mimic
will output So ignoring my personal opinion that With that said, I can see why this is useful for certain use cases and hence I suggested an approach with a helper function that omits null values from the map. This feels like the best of both worlds:
|
I know this response is coming quite late after the issue was opened but I believe Terraform 1.1 has introduced some functionality which makes this problem easier to solve. When calling a module and supplying Terraform 1.1 has released a As such, I propose that Terragrunt handles null values by passing |
Hello Terragrunt team,
Thanks for this wonderfull tool!
Currently I'm facing a blocker issue using null values. A null value variables are "unset" variables, which means that terraform does not recognize it. Said that, when a null value is assigned to a variable it should not be exported to terraform (same behavior as terraform)
Here is a thread talking about the issue: #892
One example is when a dependency dynamically offers an output and it becomes optional to the children inputs:
In the below example, when in terraform dependency's code the output was set to "null", the output actually will not be exported and will be not available, hence we will be defaulting to a null value by the try function (because has failed to find it)
That means that the input will not be set and terraform will use the default input value defined on its own "variables.tf" module's file, example:
In this case, "-1" will be used because there was no input/dependency information about this value.
Update:
I've added a new Terragrunt option for 'retro-compatibility':
--terragrunt-pass-null-vars
which will enable current behavior passing the "null" string as value for null variables