From 6f490cf3714370af40ee459ff2ef9a6cc6776e20 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 25 Jan 2024 17:14:10 -0800 Subject: [PATCH] core: Placeholder values for input variables in partial-expanded modules Since input variables are defined in the parent of the module where they are declared, this particular case ends up straddling over a module boundary: the EvalContext belongs to the parent while the node's own absolute address belongs to the child. The DynamicExpand implementation of the expander node already knows how to wire that up (the same way as for the fully-expanded equivalent) and so we don't need to do anything particularly special in this function, but it does mean that we need to provide some placeholder repetition data to use in case the definition expression refers to each.key, each.value, or count.index. For now we're using the "totally-unknown" repetition data that just sets all three symbols to unknown values. In future it would be nice to improve the precision here so that e.g. each.value can be treated as invalid when used in a call that doesn't set for_each, but that would require propagating the call configuration into this graph node and we don't yet have the needed information in the code that generates this node, so we'll save the more disruptive rework to allow that for a later change. --- internal/terraform/node_module_variable.go | 51 +++++++++++++++++----- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/internal/terraform/node_module_variable.go b/internal/terraform/node_module_variable.go index 11a053e77224..bac9682118ab 100644 --- a/internal/terraform/node_module_variable.go +++ b/internal/terraform/node_module_variable.go @@ -160,6 +160,11 @@ type nodeModuleVariable struct { // ModuleCallArguments, ex. so count.index and each.key can resolve ModuleInstance addrs.ModuleInstance + // ModuleCallConfig is the module call that the expression in field Expr + // came from, which helps decide what [instances.RepetitionData] we should + // use when evaluating Expr. + ModuleCallConfig *configs.ModuleCall + // DestroyApply must be set to true when applying a destroy operation and // false otherwise. DestroyApply bool @@ -241,12 +246,6 @@ func (n *nodeModuleVariable) DotNode(name string, opts *dag.DotOpts) *dag.DotNod // evalModuleVariable produces the value for a particular variable as will // be used by a child module instance. // -// The result is written into a map, with its key set to the local name of the -// variable, disregarding the module instance address. A map is returned instead -// of a single value as a result of trying to be convenient for use with -// EvalContext.SetModuleCallArguments, which expects a map to merge in with any -// existing arguments. -// // validateOnly indicates that this evaluation is only for config // validation, and we will not have any expansion module instance // repetition data. @@ -261,11 +260,10 @@ func (n *nodeModuleVariable) evalModuleVariable(ctx EvalContext, validateOnly bo case validateOnly: // the instance expander does not track unknown expansion values, so we // have to assume all RepetitionData is unknown. - moduleInstanceRepetitionData = instances.RepetitionData{ - CountIndex: cty.UnknownVal(cty.Number), - EachKey: cty.UnknownVal(cty.String), - EachValue: cty.DynamicVal, - } + // TODO: Ideally we should vary the placeholder we use here based + // on how the module call repetition was configured, but we don't + // have enough information here to decide that. + moduleInstanceRepetitionData = instances.TotallyUnknownRepetitionData default: // Get the repetition data for this module instance, @@ -327,4 +325,33 @@ func (n *nodeModuleVariableInPartialModule) Path() addrs.PartialExpandedModule { return n.Addr.Module } -// TODO: Implement nodeModuleVariableInPartialModule.Execute +func (n *nodeModuleVariableInPartialModule) Execute(ctx EvalContext, op walkOperation) tfdiags.Diagnostics { + // Our job here is to make sure that the input variable definition is + // valid for all instances of this input variable across all of the possible + // module instances under our partially-expanded prefix, and to record + // a placeholder value that captures as precisely as possible what all + // of those results have in common. In the worst case where they have + // absolutely nothing in common cty.DynamicVal is the ultimate fallback, + // but we should try to do better when possible to give operators earlier + // feedback about any problems they would definitely encounter on a + // subsequent plan where the input variables get evaluated concretely. + + namedVals := ctx.NamedValues() + + // TODO: Ideally we should vary the placeholder we use here based + // on how the module call repetition was configured, but we don't + // have enough information here to decide that. + moduleInstanceRepetitionData := instances.TotallyUnknownRepetitionData + + // NOTE WELL: Input variables are a bit strange in that they announce + // themselves as belonging to the caller of the module they are defined + // in, because that's where their definition expressions get evaluated. + // Therefore this [EvalContext] is in the scope of the parent module, + // while n.Addr describes an object in the child module (where the + // variable declaration appeared. + scope := ctx.EvaluationScope(nil, nil, moduleInstanceRepetitionData) + val, diags := scope.EvalExpr(n.Expr, cty.DynamicPseudoType) + + namedVals.SetInputVariablePlaceholder(n.Addr, val) + return diags +}