From f3ec86b17bb4301acbd417b87245fe2fef85c8d7 Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 7 Mar 2024 15:49:39 -0500 Subject: [PATCH] stackruntime: Treat unset and null equally When handling root input variable values, we now consider unset and null values to be equivalent to each other. This is consistent with how we handle variables in embedded stacks, and very similar to how we handle variable in the modules runtime with `nullable = false`. One difference from the modules runtime case is that we do not prevent a null default value for stack variables. --- .../internal/stackeval/input_variable.go | 7 +- .../stackruntime/internal/stackeval/main.go | 8 +- internal/stacks/stackruntime/plan_test.go | 85 +++++++++++++++++++ .../child/child.tfstack.hcl | 9 ++ .../deployments.tfdeploy.hcl | 3 + .../plan-variable-default.tfstack.hcl | 31 +++++++ internal/stacks/stackruntime/validate_test.go | 1 + 7 files changed, 137 insertions(+), 7 deletions(-) create mode 100644 internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/child/child.tfstack.hcl create mode 100644 internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/deployments.tfdeploy.hcl create mode 100644 internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/plan-variable-default.tfstack.hcl diff --git a/internal/stacks/stackruntime/internal/stackeval/input_variable.go b/internal/stacks/stackruntime/internal/stackeval/input_variable.go index 68e0482baf..da9c6a4596 100644 --- a/internal/stacks/stackruntime/internal/stackeval/input_variable.go +++ b/internal/stacks/stackruntime/internal/stackeval/input_variable.go @@ -113,9 +113,10 @@ func (v *InputVariable) CheckValue(ctx context.Context, phase EvalPhase) (cty.Va extVal := v.main.RootVariableValue(ctx, v.Addr().Item, phase) - // If the calling context does not define a value for this - // variable, we need to fall back to the default. - if extVal.Value == cty.NilVal { + // We treat a null value as equivalent to an unspecified value, + // and replace it with the variable's default value. This is + // consistent with how embedded stacks handle defaults. + if extVal.Value.IsNull() { cfg := v.Config(ctx) // A separate code path will validate the default value, so diff --git a/internal/stacks/stackruntime/internal/stackeval/main.go b/internal/stacks/stackruntime/internal/stackeval/main.go index e2772bc76b..bd10ae6ff3 100644 --- a/internal/stacks/stackruntime/internal/stackeval/main.go +++ b/internal/stacks/stackruntime/internal/stackeval/main.go @@ -455,11 +455,11 @@ func (m *Main) RootVariableValue(ctx context.Context, addr stackaddrs.InputVaria ret, ok := m.planning.opts.InputVariableValues[addr] if !ok { // If no value is specified for the given input variable, we return - // a nil placeholder. Nil can never be specified, so the caller can - // determine that the variable's default value should be used (if - // present) or an error raised (if not). + // a null value. Callers should treat a null value as equivalent to + // an unspecified one, applying default (if present) or raising an + // error (if not). return ExternalInputValue{ - Value: cty.NilVal, + Value: cty.NullVal(cty.DynamicPseudoType), } } return ret diff --git a/internal/stacks/stackruntime/plan_test.go b/internal/stacks/stackruntime/plan_test.go index 7fd9f5f5d8..b7910e6897 100644 --- a/internal/stacks/stackruntime/plan_test.go +++ b/internal/stacks/stackruntime/plan_test.go @@ -285,6 +285,91 @@ func TestPlanWithNoValueForRequiredVariable(t *testing.T) { } } +func TestPlanWithVariableDefaults(t *testing.T) { + // Test that defaults are applied correctly for both unspecified input + // variables and those with an explicit null value. + testCases := map[string]struct { + inputs map[stackaddrs.InputVariable]ExternalInputValue + }{ + "unspecified": { + inputs: make(map[stackaddrs.InputVariable]ExternalInputValue), + }, + "explicit null": { + inputs: map[stackaddrs.InputVariable]ExternalInputValue{ + stackaddrs.InputVariable{Name: "beep"}: ExternalInputValue{ + Value: cty.NullVal(cty.DynamicPseudoType), + DefRange: tfdiags.SourceRange{Filename: "fake.tfstack.hcl"}, + }, + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + cfg := loadMainBundleConfigForTest(t, "plan-variable-defaults") + + changesCh := make(chan stackplan.PlannedChange, 8) + diagsCh := make(chan tfdiags.Diagnostic, 2) + req := PlanRequest{ + Config: cfg, + InputValues: tc.inputs, + } + resp := PlanResponse{ + PlannedChanges: changesCh, + Diagnostics: diagsCh, + } + + go Plan(ctx, &req, &resp) + gotChanges, diags := collectPlanOutput(changesCh, diagsCh) + + if len(diags) != 0 { + t.Errorf("unexpected diagnostics\n%s", diags.ErrWithWarnings().Error()) + } + + wantChanges := []stackplan.PlannedChange{ + &stackplan.PlannedChangeApplyable{ + Applyable: true, + }, + &stackplan.PlannedChangeHeader{ + TerraformVersion: version.SemVer, + }, + &stackplan.PlannedChangeOutputValue{ + Addr: stackaddrs.OutputValue{Name: "beep"}, + Action: plans.Create, + OldValue: plans.DynamicValue{0xc0}, // MessagePack nil + NewValue: plans.DynamicValue([]byte("\xa4BEEP")), // MessagePack string "BEEP" + }, + &stackplan.PlannedChangeOutputValue{ + Addr: stackaddrs.OutputValue{Name: "defaulted"}, + Action: plans.Create, + OldValue: plans.DynamicValue{0xc0}, // MessagePack nil + NewValue: plans.DynamicValue([]byte("\xa4BOOP")), // MessagePack string "BOOP" + }, + &stackplan.PlannedChangeOutputValue{ + Addr: stackaddrs.OutputValue{Name: "specified"}, + Action: plans.Create, + OldValue: plans.DynamicValue{0xc0}, // MessagePack nil + NewValue: plans.DynamicValue([]byte("\xa4BEEP")), // MessagePack string "BEEP" + }, + &stackplan.PlannedChangeRootInputValue{ + Addr: stackaddrs.InputVariable{ + Name: "beep", + }, + Value: cty.StringVal("BEEP"), + }, + } + sort.SliceStable(gotChanges, func(i, j int) bool { + return plannedChangeSortKey(gotChanges[i]) < plannedChangeSortKey(gotChanges[j]) + }) + + if diff := cmp.Diff(wantChanges, gotChanges, ctydebug.CmpOptions); diff != "" { + t.Errorf("wrong changes\n%s", diff) + } + }) + } +} + func TestPlanWithSingleResource(t *testing.T) { ctx := context.Background() cfg := loadMainBundleConfigForTest(t, "with-single-resource") diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/child/child.tfstack.hcl b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/child/child.tfstack.hcl new file mode 100644 index 0000000000..7fa976ffda --- /dev/null +++ b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/child/child.tfstack.hcl @@ -0,0 +1,9 @@ +variable "boop" { + type = string + default = "BOOP" +} + +output "result" { + type = string + value = var.boop +} \ No newline at end of file diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/deployments.tfdeploy.hcl b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/deployments.tfdeploy.hcl new file mode 100644 index 0000000000..811bbfdf56 --- /dev/null +++ b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/deployments.tfdeploy.hcl @@ -0,0 +1,3 @@ +deployment "main" { + inputs = {} +} diff --git a/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/plan-variable-default.tfstack.hcl b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/plan-variable-default.tfstack.hcl new file mode 100644 index 0000000000..37d763eeef --- /dev/null +++ b/internal/stacks/stackruntime/testdata/mainbundle/test/plan-variable-defaults/plan-variable-default.tfstack.hcl @@ -0,0 +1,31 @@ +variable "beep" { + type = string + default = "BEEP" +} + +output "beep" { + type = string + value = var.beep +} + +stack "specified" { + source = "./child" + inputs = { + boop = var.beep + } +} + +stack "defaulted" { + source = "./child" + inputs = {} +} + +output "specified" { + type = string + value = stack.specified.result +} + +output "defaulted" { + type = string + value = stack.defaulted.result +} diff --git a/internal/stacks/stackruntime/validate_test.go b/internal/stacks/stackruntime/validate_test.go index 64cd295dc2..63564a6770 100644 --- a/internal/stacks/stackruntime/validate_test.go +++ b/internal/stacks/stackruntime/validate_test.go @@ -38,6 +38,7 @@ var ( // validConfigurations are shared between the validate and plan tests. validConfigurations = map[string]validateTestInput{ "empty": {}, + "plan-variable-defaults": {}, "variable-output-roundtrip": {}, "variable-output-roundtrip-nested": {}, filepath.Join("with-single-input", "input-from-component"): {},