From 564b57b1f64f618da52739bc741caf5dccecec56 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 7 Oct 2019 15:24:16 -0700 Subject: [PATCH] core: Require variables correctly set during NewContext We previously deferred this to Validate, but all of our operations require a valid set of variables and so checking this up front makes it more obvious when a call into Terraform Core from the CLI layer isn't populating variables correctly, instead of having it fail downstream somewhere. It's the caller's responsibility to ensure that it has collected values for all of the variables in some way before calling NewContext, because in the main case driven by the CLI there are many different places that variable values can be collected from and so handling the main user-facing validation in the CLI allows us to return better error messages that take into account which way a variable is (or is not) being set. --- terraform/context.go | 22 +++++++++++++--------- terraform/context_apply_test.go | 12 ------------ terraform/context_plan_test.go | 16 ++++++++-------- terraform/context_validate_test.go | 27 +++++++++------------------ 4 files changed, 30 insertions(+), 47 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 76be6dfc5..b0162f6e4 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -210,6 +210,18 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) { log.Printf("[TRACE] terraform.NewContext: complete") + // By the time we get here, we should have values defined for all of + // the root module variables, even if some of them are "unknown". It's the + // caller's responsibility to have already handled the decoding of these + // from the various ways the CLI allows them to be set and to produce + // user-friendly error messages if they are not all present, and so + // the error message from checkInputVariables should never be seen and + // includes language asking the user to report a bug. + if config != nil { + varDiags := checkInputVariables(config.Module.Variables, variables) + diags = diags.Append(varDiags) + } + return &Context{ components: components, schemas: schemas, @@ -227,7 +239,7 @@ func NewContext(opts *ContextOpts) (*Context, tfdiags.Diagnostics) { providerInputConfig: make(map[string]map[string]cty.Value), providerSHA256s: opts.ProviderSHA256s, sh: sh, - }, nil + }, diags } func (c *Context) Schemas() *Schemas { @@ -658,14 +670,6 @@ func (c *Context) Validate() tfdiags.Diagnostics { var diags tfdiags.Diagnostics - // Validate input variables. We do this only for the values supplied - // by the root module, since child module calls are validated when we - // visit their graph nodes. - if c.config != nil { - varDiags := checkInputVariables(c.config.Module.Variables, c.variables) - diags = diags.Append(varDiags) - } - // If we have errors at this point then we probably won't be able to // construct a graph without producing redundant errors, so we'll halt early. if diags.HasErrors() { diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 808e62bbc..203bd015d 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -4803,12 +4803,6 @@ func TestContext2Apply_provisionerFail(t *testing.T) { Provisioners: map[string]ProvisionerFactory{ "shell": testProvisionerFuncFixed(pr), }, - Variables: InputValues{ - "value": &InputValue{ - Value: cty.NumberIntVal(1), - SourceType: ValueFromCaller, - }, - }, }) if _, diags := ctx.Plan(); diags.HasErrors() { @@ -6650,12 +6644,6 @@ func TestContext2Apply_destroyModuleWithAttrsReferencingResource(t *testing.T) { "aws": testProviderFuncFixed(p), }, ), - Variables: InputValues{ - "key_name": &InputValue{ - Value: cty.StringVal("foobarkey"), - SourceType: ValueFromCaller, - }, - }, }) // First plan and apply a create operation diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index edb382860..7ca47f778 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -213,12 +213,6 @@ func TestContext2Plan_createBefore_maintainRoot(t *testing.T) { "aws": testProviderFuncFixed(p), }, ), - Variables: InputValues{ - "in": &InputValue{ - Value: cty.StringVal("a,b,c"), - SourceType: ValueFromCaller, - }, - }, }) plan, diags := ctx.Plan() @@ -3388,8 +3382,8 @@ func TestContext2Plan_forEach(t *testing.T) { } func TestContext2Plan_forEachUnknownValue(t *testing.T) { - // This module has a variable defined, but it is not provided - // in the context below and we expect the plan to error, but not panic + // This module has a variable defined, but it's value is unknown. We + // expect this to produce an error, but not to panic. m := testModule(t, "plan-for-each-unknown-value") p := testProvider("aws") p.DiffFn = testDiffFn @@ -3400,6 +3394,12 @@ func TestContext2Plan_forEachUnknownValue(t *testing.T) { "aws": testProviderFuncFixed(p), }, ), + Variables: InputValues{ + "foo": { + Value: cty.UnknownVal(cty.String), + SourceType: ValueFromCLIArg, + }, + }, }) _, diags := ctx.Plan() diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index aae8da090..5557fa67b 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -87,35 +87,28 @@ func TestContext2Validate_varMapOverrideOld(t *testing.T) { }, } - c := testContext2(t, &ContextOpts{ + _, diags := NewContext(&ContextOpts{ Config: m, ProviderResolver: providers.ResolverFixed( map[string]providers.Factory{ "aws": testProviderFuncFixed(p), }, ), - Variables: InputValues{ - "foo.foo": &InputValue{ - Value: cty.StringVal("bar"), - SourceType: ValueFromCaller, - }, - }, + Variables: InputValues{}, }) - - diags := c.Validate() if !diags.HasErrors() { + // Error should be: The input variable "provider_var" has not been assigned a value. t.Fatalf("succeeded; want error") } } func TestContext2Validate_varNoDefaultExplicitType(t *testing.T) { m := testModule(t, "validate-var-no-default-explicit-type") - c := testContext2(t, &ContextOpts{ + _, diags := NewContext(&ContextOpts{ Config: m, }) - - diags := c.Validate() if !diags.HasErrors() { + // Error should be: The input variable "maybe_a_map" has not been assigned a value. t.Fatalf("succeeded; want error") } } @@ -314,7 +307,7 @@ func TestContext2Validate_countVariableNoDefault(t *testing.T) { }, } - c := testContext2(t, &ContextOpts{ + _, diags := NewContext(&ContextOpts{ Config: m, ProviderResolver: providers.ResolverFixed( map[string]providers.Factory{ @@ -322,9 +315,8 @@ func TestContext2Validate_countVariableNoDefault(t *testing.T) { }, ), }) - - diags := c.Validate() if !diags.HasErrors() { + // Error should be: The input variable "foo" has not been assigned a value. t.Fatalf("succeeded; want error") } } @@ -847,7 +839,7 @@ func TestContext2Validate_requiredVar(t *testing.T) { }, } - c := testContext2(t, &ContextOpts{ + _, diags := NewContext(&ContextOpts{ Config: m, ProviderResolver: providers.ResolverFixed( map[string]providers.Factory{ @@ -855,9 +847,8 @@ func TestContext2Validate_requiredVar(t *testing.T) { }, ), }) - - diags := c.Validate() if !diags.HasErrors() { + // Error should be: The input variable "foo" has not been assigned a value. t.Fatalf("succeeded; want error") } }