-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Type Constraints for Output Blocks #36411
base: main
Are you sure you want to change the base?
Conversation
6b45389
to
813f9e4
Compare
output "object" { | ||
type = object({ | ||
name = optional(string, "Bart"), | ||
}) | ||
value = {} | ||
} |
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.
Should we support both explicit and implicit default values?
This already introduces support for implicit defaults through optional attributes. While I initially thought against defaults entirely, they likely provide real value to users. For consistency sake,
Perhaps we should support both patterns rather than just one.
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'm not sure if I understand what you're asking here. How would an explicit default value work in the context of an output block?
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 was thinking we could assign the default when the assigned attribute is null,
like we have for variables https://developer.hashicorp.com/terraform/language/values/variables#default-values
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.
Does this mean you're proposing to introduce a new default
attribute for output blocks? I think we would have to discuss whether that makes sense from a language point of view.
Unlike variables, outputs usually get their values from configuration and don't depend on user-supplied values. Where a user might only supply some of the variables, but not all, we fall back on the default. An output should always have a value, even if it is sometimes null
.
The optional
modifier in the above test case only works within objects https://developer.hashicorp.com/terraform/language/expressions/type-constraints#optional-object-type-attributes
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.
Yes, I was proposing a new default
attribute for outputs. Your explanation is very helpful, thank you.
An output should always have a value, even if it is sometimes null.
Make sense - since outputs are guaranteed to have a value (with null
being a valid value), then there's no need for default handling.
When this is being documented, I think it would however make sense to add some documentation clarifying this distinction between variables and outputs, particularly around null handling? This might help other users avoid similar misconceptions.
if defaults != nil { | ||
val = defaults.Apply(val) | ||
} |
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.
We may want to follow the same rule as here, and check that the value isn't null before applying the default.
terraform/internal/terraform/eval_variable.go
Lines 99 to 105 in 6676eef
// Apply defaults from the variable's type constraint to the converted value, | |
// unless the converted value is null. We do not apply defaults to top-level | |
// null values, as doing so could prevent assigning null to a nullable | |
// variable. | |
if cfg.TypeDefaults != nil && !given.IsNull() { | |
given = cfg.TypeDefaults.Apply(given) | |
} |
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.
This doesn't apply to outputs, because only input variables have a weird "nullable" behavior.
A mistake was made in the initial implementation of module variables, causing them to work differently than other assignments in the language. The language declares that null
is the absence of a value, but assigning null
to a variable input overrode the default, so we had to add a nullable
option to input variables to get the correct behavior.
// TODO: Populate EvalContext and Expression, but we can't do that | ||
// as long as we're using the ctx.EvaluateExpr helper above because | ||
// the EvalContext is hidden from us in that case. | ||
}) |
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.
Prior art here
terraform/internal/terraform/eval_for_each.go
Lines 176 to 185 in b701e15
refs, moreDiags := langrefs.ReferencesInExpr(addrs.ParseRef, ev.expr) | |
diags = diags.Append(moreDiags) | |
scope := ev.ctx.EvaluationScope(nil, nil, EvalDataForNoInstanceKey) | |
if scope != nil { | |
ev.hclCtx, moreDiags = scope.EvalContext(refs) | |
} else { | |
// This shouldn't happen in real code, but it can unfortunately arise | |
// in unit tests due to incompletely-implemented mocks. :( | |
ev.hclCtx = &hcl.EvalContext{} | |
} |
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.
What do you think of this comment? If we are releasing this as an experiment, we likely do not need to do that yet. However, it is to be released in the stable version, we may want to consider extending the constraints to module calls.
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.
Excellent question! I brought this up in our team jam on Tuesday and @jbardin raised a concern with this approach: If a practitioner has a module with fully typed outputs and later adds an output without type, Terraform will suddenly evaluate the module differently. This may cause more confusion than it helps the practitioner. But I have no strong opinion on this, and would be happy to discuss this further with you both.
I assume this could be one place where we could make use of the type information
terraform/internal/terraform/evaluate.go
Lines 387 to 402 in 813f9e4
if d.Operation == walkValidate { | |
atys := make(map[string]cty.Type, len(outputConfigs)) | |
for name := range outputConfigs { | |
atys[name] = cty.DynamicPseudoType // output values are dynamically-typed | |
} | |
instTy := cty.Object(atys) | |
switch { | |
case callConfig.Count != nil: | |
return cty.UnknownVal(cty.List(instTy)), diags | |
case callConfig.ForEach != nil: | |
return cty.UnknownVal(cty.Map(instTy)), diags | |
default: | |
return cty.UnknownVal(instTy), diags | |
} | |
} |
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.
Thank you. That concern probably makes sense, but still not entirely clear to me. I'd love to discuss further too
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.
Did not mean to approve 😅
// setting vs. just using the default type constraint when processing | ||
// override files. | ||
TypeSet bool |
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.
Is it possible to create a test showing the use of override files to override an output with type constraints? I can imagine how this bool would be useful in that case, but it looks like this field is set but unused currently.
This PR introduces support for declaring explicit type constraints in output blocks. It picks up the work that Martin did some time ago in #31728.
We could also introduce this as an experiment, available only in alpha releases, if we want to learn if a new argument inside the output block is the most ideal design for it.
UX
Constraint mismatches are raised as an error
Fixes #
Target Release
1.12.x
CHANGELOG entry