From 1226e77999c52ea30685d9e0224298fffd3d298a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 20 Nov 2018 12:17:57 -0800 Subject: [PATCH] core: Remove GraphSemanticChecker, etc These overly-general interfaces are no longer used anywhere, and their presence in the important-sounding semantics.go file was a distracting red herring. We'd previously replaced the one checker in here with a simple helper function for checking input variables, and that's arguably more at home with all of the other InputValue functionality in variables.go, and that allows us to remove semantics.go (and its associated test file) altogether and make room for some forthcoming new files for static validation. --- terraform/semantics.go | 137 ------------------------------------ terraform/semantics_test.go | 50 ------------- terraform/variables.go | 90 ++++++++++++++++++++++- terraform/variables_test.go | 43 +++++++++++ 4 files changed, 132 insertions(+), 188 deletions(-) delete mode 100644 terraform/semantics.go delete mode 100644 terraform/semantics_test.go diff --git a/terraform/semantics.go b/terraform/semantics.go deleted file mode 100644 index 3f6189a98..000000000 --- a/terraform/semantics.go +++ /dev/null @@ -1,137 +0,0 @@ -package terraform - -import ( - "fmt" - - "github.com/hashicorp/go-multierror" - "github.com/hashicorp/hcl2/hcl" - "github.com/hashicorp/terraform/configs" - "github.com/hashicorp/terraform/dag" - "github.com/hashicorp/terraform/tfdiags" - "github.com/zclconf/go-cty/cty/convert" -) - -// GraphSemanticChecker is the interface that semantic checks across -// the entire Terraform graph implement. -// -// The graph should NOT be modified by the semantic checker. -type GraphSemanticChecker interface { - Check(*dag.Graph) error -} - -// UnorderedSemanticCheckRunner is an implementation of GraphSemanticChecker -// that runs a list of SemanticCheckers against the vertices of the graph -// in no specified order. -type UnorderedSemanticCheckRunner struct { - Checks []SemanticChecker -} - -func (sc *UnorderedSemanticCheckRunner) Check(g *dag.Graph) error { - var err error - for _, v := range g.Vertices() { - for _, check := range sc.Checks { - if e := check.Check(g, v); e != nil { - err = multierror.Append(err, e) - } - } - } - - return err -} - -// SemanticChecker is the interface that semantic checks across the -// Terraform graph implement. Errors are accumulated. Even after an error -// is returned, child vertices in the graph will still be visited. -// -// The graph should NOT be modified by the semantic checker. -// -// The order in which vertices are visited is left unspecified, so the -// semantic checks should not rely on that. -type SemanticChecker interface { - Check(*dag.Graph, dag.Vertex) error -} - -// checkInputVariables ensures that variable values supplied at the UI conform -// to their corresponding declarations in configuration. -// -// The set of values is considered valid only if the returned diagnostics -// does not contain errors. A valid set of values may still produce warnings, -// which should be returned to the user. -func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdiags.Diagnostics { - var diags tfdiags.Diagnostics - - for name, vc := range vcs { - val, isSet := vs[name] - if !isSet { - // Always an error, since the caller should already have included - // default values from the configuration in the values map. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Unassigned variable", - fmt.Sprintf("The input variable %q has not been assigned a value. This is a bug in Terraform; please report it in a GitHub issue.", name), - )) - continue - } - - wantType := vc.Type - - // A given value is valid if it can convert to the desired type. - _, err := convert.Convert(val.Value, wantType) - if err != nil { - switch val.SourceType { - case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: - // We have source location information for these. - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid value for input variable", - Detail: fmt.Sprintf("The given value is not valid for variable %q: %s.", name, err), - Subject: val.SourceRange.ToHCL().Ptr(), - }) - case ValueFromEnvVar: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The environment variable TF_VAR_%s does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromCLIArg: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The argument -var=\"%s=...\" does not contain a valid value for variable %q: %s.", name, name, err), - )) - case ValueFromInput: - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value entered for variable %q is not valid: %s.", name, err), - )) - default: - // The above gets us good coverage for the situations users - // are likely to encounter with their own inputs. The other - // cases are generally implementation bugs, so we'll just - // use a generic error for these. - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Invalid value for input variable", - fmt.Sprintf("The value provided for variable %q is not valid: %s.", name, err), - )) - } - } - } - - // Check for any variables that are assigned without being configured. - // This is always an implementation error in the caller, because we - // expect undefined variables to be caught during context construction - // where there is better context to report it well. - for name := range vs { - if _, defined := vcs[name]; !defined { - diags = diags.Append(tfdiags.Sourceless( - tfdiags.Error, - "Value assigned to undeclared variable", - fmt.Sprintf("A value was assigned to an undeclared input variable %q.", name), - )) - } - } - - return diags -} diff --git a/terraform/semantics_test.go b/terraform/semantics_test.go deleted file mode 100644 index 46346f2d3..000000000 --- a/terraform/semantics_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package terraform - -import ( - "testing" - - "github.com/zclconf/go-cty/cty" -) - -func TestSMCUserVariables(t *testing.T) { - c := testModule(t, "smc-uservars") - - // No variables set - diags := checkInputVariables(c.Module.Variables, nil) - if !diags.HasErrors() { - t.Fatal("check succeeded, but want errors") - } - - // Required variables set, optional variables unset - // This is still an error at this layer, since it's the caller's - // responsibility to have already merged in any default values. - diags = checkInputVariables(c.Module.Variables, InputValues{ - "foo": &InputValue{ - Value: cty.StringVal("bar"), - SourceType: ValueFromCLIArg, - }, - }) - if !diags.HasErrors() { - t.Fatal("check succeeded, but want errors") - } - - // All variables set - diags = checkInputVariables(c.Module.Variables, InputValues{ - "foo": &InputValue{ - Value: cty.StringVal("bar"), - SourceType: ValueFromCLIArg, - }, - "bar": &InputValue{ - Value: cty.StringVal("baz"), - SourceType: ValueFromCLIArg, - }, - "map": &InputValue{ - Value: cty.StringVal("baz"), // okay because config has no type constraint - SourceType: ValueFromCLIArg, - }, - }) - if diags.HasErrors() { - //t.Fatal("check succeeded, but want errors") - t.Fatalf("unexpected errors: %s", diags.Err()) - } -} diff --git a/terraform/variables.go b/terraform/variables.go index e54e27563..75531b2bf 100644 --- a/terraform/variables.go +++ b/terraform/variables.go @@ -3,9 +3,12 @@ package terraform import ( "fmt" + "github.com/hashicorp/hcl2/hcl" + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/convert" + "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/tfdiags" - "github.com/zclconf/go-cty/cty" ) // InputValue represents a value for a variable in the root module, provided @@ -223,3 +226,88 @@ func (vv InputValues) Identical(other InputValues) bool { return true } + +// checkInputVariables ensures that variable values supplied at the UI conform +// to their corresponding declarations in configuration. +// +// The set of values is considered valid only if the returned diagnostics +// does not contain errors. A valid set of values may still produce warnings, +// which should be returned to the user. +func checkInputVariables(vcs map[string]*configs.Variable, vs InputValues) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + for name, vc := range vcs { + val, isSet := vs[name] + if !isSet { + // Always an error, since the caller should already have included + // default values from the configuration in the values map. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Unassigned variable", + fmt.Sprintf("The input variable %q has not been assigned a value. This is a bug in Terraform; please report it in a GitHub issue.", name), + )) + continue + } + + wantType := vc.Type + + // A given value is valid if it can convert to the desired type. + _, err := convert.Convert(val.Value, wantType) + if err != nil { + switch val.SourceType { + case ValueFromConfig, ValueFromAutoFile, ValueFromNamedFile: + // We have source location information for these. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid value for input variable", + Detail: fmt.Sprintf("The given value is not valid for variable %q: %s.", name, err), + Subject: val.SourceRange.ToHCL().Ptr(), + }) + case ValueFromEnvVar: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid value for input variable", + fmt.Sprintf("The environment variable TF_VAR_%s does not contain a valid value for variable %q: %s.", name, name, err), + )) + case ValueFromCLIArg: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid value for input variable", + fmt.Sprintf("The argument -var=\"%s=...\" does not contain a valid value for variable %q: %s.", name, name, err), + )) + case ValueFromInput: + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid value for input variable", + fmt.Sprintf("The value entered for variable %q is not valid: %s.", name, err), + )) + default: + // The above gets us good coverage for the situations users + // are likely to encounter with their own inputs. The other + // cases are generally implementation bugs, so we'll just + // use a generic error for these. + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Invalid value for input variable", + fmt.Sprintf("The value provided for variable %q is not valid: %s.", name, err), + )) + } + } + } + + // Check for any variables that are assigned without being configured. + // This is always an implementation error in the caller, because we + // expect undefined variables to be caught during context construction + // where there is better context to report it well. + for name := range vs { + if _, defined := vcs[name]; !defined { + diags = diags.Append(tfdiags.Sourceless( + tfdiags.Error, + "Value assigned to undeclared variable", + fmt.Sprintf("A value was assigned to an undeclared input variable %q.", name), + )) + } + } + + return diags +} diff --git a/terraform/variables_test.go b/terraform/variables_test.go index da0ae915e..0aaed3aa6 100644 --- a/terraform/variables_test.go +++ b/terraform/variables_test.go @@ -162,3 +162,46 @@ func TestVariables(t *testing.T) { }) } } + +func TestSMCUserVariables(t *testing.T) { + c := testModule(t, "smc-uservars") + + // No variables set + diags := checkInputVariables(c.Module.Variables, nil) + if !diags.HasErrors() { + t.Fatal("check succeeded, but want errors") + } + + // Required variables set, optional variables unset + // This is still an error at this layer, since it's the caller's + // responsibility to have already merged in any default values. + diags = checkInputVariables(c.Module.Variables, InputValues{ + "foo": &InputValue{ + Value: cty.StringVal("bar"), + SourceType: ValueFromCLIArg, + }, + }) + if !diags.HasErrors() { + t.Fatal("check succeeded, but want errors") + } + + // All variables set + diags = checkInputVariables(c.Module.Variables, InputValues{ + "foo": &InputValue{ + Value: cty.StringVal("bar"), + SourceType: ValueFromCLIArg, + }, + "bar": &InputValue{ + Value: cty.StringVal("baz"), + SourceType: ValueFromCLIArg, + }, + "map": &InputValue{ + Value: cty.StringVal("baz"), // okay because config has no type constraint + SourceType: ValueFromCLIArg, + }, + }) + if diags.HasErrors() { + //t.Fatal("check succeeded, but want errors") + t.Fatalf("unexpected errors: %s", diags.Err()) + } +}