-
Notifications
You must be signed in to change notification settings - Fork 10k
Type Constraints for Output Blocks #36411
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
kind: ENHANCEMENTS | ||
body: 'config: `output` blocks now can have an explicit type constraints' | ||
time: 2025-02-03T17:58:07.110141+01:00 | ||
custom: | ||
Issue: "36411" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -346,11 +346,23 @@ type Output struct { | |
Sensitive bool | ||
Ephemeral bool | ||
|
||
// ConstraintType is a type constraint which the result is guaranteed | ||
// to conform to when used in the calling module. | ||
ConstraintType cty.Type | ||
// TypeDefaults describes any optional attribute defaults that should be | ||
// applied to the Expr result before type conversion. | ||
TypeDefaults *typeexpr.Defaults | ||
|
||
Preconditions []*CheckRule | ||
|
||
DescriptionSet bool | ||
SensitiveSet bool | ||
EphemeralSet bool | ||
// TypeSet is true if there was an explicit "type" argument in the | ||
// configuration block. This is mainly to allow distinguish explicitly | ||
// setting vs. just using the default type constraint when processing | ||
// override files. | ||
TypeSet bool | ||
Comment on lines
+363
to
+365
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
DeclRange hcl.Range | ||
} | ||
|
@@ -390,6 +402,19 @@ func decodeOutputBlock(block *hcl.Block, override bool) (*Output, hcl.Diagnostic | |
o.Expr = attr.Expr | ||
} | ||
|
||
if attr, exists := content.Attributes["type"]; exists { | ||
ty, defaults, moreDiags := typeexpr.TypeConstraintWithDefaults(attr.Expr) | ||
diags = append(diags, moreDiags...) | ||
o.ConstraintType = ty | ||
o.TypeDefaults = defaults | ||
o.TypeSet = true | ||
} | ||
if o.ConstraintType == cty.NilType { | ||
// If no constraint is given then the type will be inferred | ||
// automatically from the value. | ||
o.ConstraintType = cty.DynamicPseudoType | ||
} | ||
|
||
if attr, exists := content.Attributes["sensitive"]; exists { | ||
valDiags := gohcl.DecodeExpression(attr.Expr, nil, &o.Sensitive) | ||
diags = append(diags, valDiags...) | ||
|
@@ -525,6 +550,9 @@ var outputBlockSchema = &hcl.BodySchema{ | |
{ | ||
Name: "ephemeral", | ||
}, | ||
{ | ||
Name: "type", | ||
}, | ||
}, | ||
Blocks: []hcl.BlockHeaderSchema{ | ||
{Type: "precondition"}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
output "string" { | ||
type = string | ||
value = "Hello" | ||
} | ||
|
||
output "object" { | ||
type = object({ | ||
name = optional(string, "Bart"), | ||
}) | ||
value = {} | ||
} | ||
Comment on lines
+6
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean you're proposing to introduce a new 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 The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was proposing a new
Make sense - since outputs are guaranteed to have a value (with 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. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,9 @@ import ( | |||||||||||||||||||||
"log" | ||||||||||||||||||||||
|
||||||||||||||||||||||
"github.com/hashicorp/hcl/v2" | ||||||||||||||||||||||
"github.com/hashicorp/hcl/v2/ext/typeexpr" | ||||||||||||||||||||||
"github.com/zclconf/go-cty/cty" | ||||||||||||||||||||||
"github.com/zclconf/go-cty/cty/convert" | ||||||||||||||||||||||
|
||||||||||||||||||||||
"github.com/hashicorp/terraform/internal/addrs" | ||||||||||||||||||||||
"github.com/hashicorp/terraform/internal/configs" | ||||||||||||||||||||||
|
@@ -436,7 +438,7 @@ func (n *NodeApplyableOutput) Execute(ctx EvalContext, op walkOperation) (diags | |||||||||||||||||||||
// This has to run before we have a state lock, since evaluation also | ||||||||||||||||||||||
// reads the state | ||||||||||||||||||||||
var evalDiags tfdiags.Diagnostics | ||||||||||||||||||||||
val, evalDiags = ctx.EvaluateExpr(n.Config.Expr, cty.DynamicPseudoType, nil) | ||||||||||||||||||||||
val, evalDiags = evalOutputValue(ctx, n.Config.Expr, n.Config.ConstraintType, n.Config.TypeDefaults) | ||||||||||||||||||||||
diags = diags.Append(evalDiags) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// We'll handle errors below, after we have loaded the module. | ||||||||||||||||||||||
|
@@ -529,6 +531,38 @@ If you do intend to export this data, annotate the output value as sensitive by | |||||||||||||||||||||
return diags | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// evalOutputValue encapsulates the logic for transforming an author's value | ||||||||||||||||||||||
// expression into a valid value of their declared type constraint, or returning | ||||||||||||||||||||||
// an error describing why that isn't possible. | ||||||||||||||||||||||
func evalOutputValue(ctx EvalContext, expr hcl.Expression, wantType cty.Type, defaults *typeexpr.Defaults) (cty.Value, tfdiags.Diagnostics) { | ||||||||||||||||||||||
// We can't pass wantType to EvaluateExpr here because we'll need to | ||||||||||||||||||||||
// possibly apply our defaults before attempting type conversion below. | ||||||||||||||||||||||
val, diags := ctx.EvaluateExpr(expr, cty.DynamicPseudoType, nil) | ||||||||||||||||||||||
if diags.HasErrors() { | ||||||||||||||||||||||
return cty.UnknownVal(wantType), diags | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
if defaults != nil { | ||||||||||||||||||||||
val = defaults.Apply(val) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
dsa0x marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||
|
||||||||||||||||||||||
val, err := convert.Convert(val, wantType) | ||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||
diags = diags.Append(&hcl.Diagnostic{ | ||||||||||||||||||||||
Severity: hcl.DiagError, | ||||||||||||||||||||||
Summary: "Invalid output value", | ||||||||||||||||||||||
Detail: fmt.Sprintf("The value expression does not match this output value's type constraint: %s.", tfdiags.FormatError(err)), | ||||||||||||||||||||||
Subject: expr.Range().Ptr(), | ||||||||||||||||||||||
// 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. | ||||||||||||||||||||||
}) | ||||||||||||||||||||||
Comment on lines
+556
to
+559
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||
return cty.UnknownVal(wantType), diags | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
return val, diags | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// dag.GraphNodeDotter impl. | ||||||||||||||||||||||
func (n *NodeApplyableOutput) DotNode(name string, opts *dag.DotOpts) *dag.DotNode { | ||||||||||||||||||||||
return &dag.DotNode{ | ||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
output "string" { | ||
type = string | ||
value = true | ||
} | ||
|
||
output "object_default" { | ||
type = object({ | ||
name = optional(string, "Bart") | ||
}) | ||
value = {} | ||
} | ||
|
||
output "object_override" { | ||
type = object({ | ||
name = optional(string, "Bart") | ||
}) | ||
value = { | ||
name = "Lisa" | ||
} | ||
} |
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
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