From 6d8cce501e25efae057362eed8d7f1e4adae35e7 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 9 Mar 2020 16:16:59 +0100 Subject: [PATCH] tweak validation & add tests --- hcl2template/parser.go | 2 +- hcl2template/testdata/variables/empty.pkr.hcl | 0 hcl2template/types.packer_config.go | 6 ++ hcl2template/types.variables.go | 22 ++++- hcl2template/types.variables_test.go | 85 ++++++++++++++----- 5 files changed, 89 insertions(+), 26 deletions(-) create mode 100644 hcl2template/testdata/variables/empty.pkr.hcl diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 9ecbde9a2..df6ef1d86 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -123,7 +123,7 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, varFiles = append(varFiles, f) } - diags = append(diags, cfg.InputVariables.collectVariableValues(os.Environ(), varFiles, vars)...) + diags = append(diags, cfg.collectInputVariableValues(os.Environ(), varFiles, vars)...) } // decode the actual content diff --git a/hcl2template/testdata/variables/empty.pkr.hcl b/hcl2template/testdata/variables/empty.pkr.hcl new file mode 100644 index 000000000..e69de29bb diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 5748237b2..d972dd90c 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -25,10 +25,16 @@ type PackerConfig struct { InputVariables Variables LocalVariables Variables + ValidationOptions + // Builds is the list of Build blocks defined in the config files. Builds Builds } +type ValidationOptions struct { + Strict bool +} + // EvalContext returns the *hcl.EvalContext that will be passed to an hcl // decoder in order to tell what is the actual value of a var or a local and // the list of defined functions. diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 55158379b..5dc5579c2 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -223,8 +223,9 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval // them. const VarEnvPrefix = "PKR_VAR_" -func (variables Variables) collectVariableValues(env []string, files []*hcl.File, argv map[string]string) hcl.Diagnostics { +func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.File, argv map[string]string) hcl.Diagnostics { var diags hcl.Diagnostics + variables := cfg.InputVariables for _, raw := range env { if !strings.HasPrefix(raw, VarEnvPrefix) { @@ -321,7 +322,20 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File for name, attr := range attrs { variable, found := variables[name] if !found { - // No file defines this variable; let's skip it + sev := hcl.DiagWarning + if cfg.ValidationOptions.Strict { + sev = hcl.DiagError + } + diags = append(diags, &hcl.Diagnostic{ + Severity: sev, + Summary: "Undefined variable", + Detail: fmt.Sprintf("A %q variable was set but was "+ + "not found in known variables. To declare "+ + "variable %q, place this block in one of your"+ + ".pkr files, such as variables.pkr.hcl", + name, name), + Context: attr.Range.Ptr(), + }) continue } @@ -351,8 +365,8 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File variable, found := variables[name] if !found { diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Unknown -var variable", + Severity: hcl.DiagError, + Summary: "Undefined -var variable", Detail: fmt.Sprintf("A %q variable was passed in the command "+ "line but was not found in known variables."+ "To declare variable %q, place this block in one of your"+ diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index b4cd03b78..7e9863284 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -247,12 +247,14 @@ func TestVariables_collectVariableValues(t *testing.T) { argv map[string]string } tests := []struct { - name string - variables Variables - args args - wantDiags bool - wantVariables Variables - wantValues map[string]cty.Value + name string + variables Variables + validationOptions ValidationOptions + args args + wantDiags bool + wantDiagsHasError bool + wantVariables Variables + wantValues map[string]cty.Value }{ {name: "string", @@ -333,11 +335,39 @@ func TestVariables_collectVariableValues(t *testing.T) { }, }, - {name: "undefined but set value", + {name: "undefined but set value - pkrvar file - normal mode", variables: Variables{}, args: args{ - env: []string{`PKR_VAR_unused_string=value`}, - hclFiles: []string{`unused_string="value"`}, + hclFiles: []string{`undefined_string="value"`}, + }, + + // output + wantDiags: true, + wantDiagsHasError: false, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, + }, + + {name: "undefined but set value - pkrvar file - strict mode", + variables: Variables{}, + validationOptions: ValidationOptions{ + Strict: true, + }, + args: args{ + hclFiles: []string{`undefined_string="value"`}, + }, + + // output + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, + }, + + {name: "undefined but set value - env", + variables: Variables{}, + args: args{ + env: []string{`PKR_VAR_undefined_string=value`}, }, // output @@ -346,18 +376,19 @@ func TestVariables_collectVariableValues(t *testing.T) { wantValues: map[string]cty.Value{}, }, - {name: "undefined but set value - args", + {name: "undefined but set value - argv", variables: Variables{}, args: args{ argv: map[string]string{ - "unused_string": "value", + "undefined_string": "value", }, }, // output - wantDiags: true, - wantVariables: Variables{}, - wantValues: map[string]cty.Value{}, + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, }, {name: "value not corresponding to type - env", @@ -371,7 +402,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -394,7 +426,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -419,7 +452,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -438,9 +472,10 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, - wantVariables: Variables{}, - wantValues: map[string]cty.Value{}, + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, }, } for _, tt := range tests { @@ -454,9 +489,17 @@ func TestVariables_collectVariableValues(t *testing.T) { } files = append(files, file) } - if gotDiags := tt.variables.collectVariableValues(tt.args.env, files, tt.args.argv); (gotDiags == nil) == tt.wantDiags { + cfg := &PackerConfig{ + InputVariables: tt.variables, + ValidationOptions: tt.validationOptions, + } + gotDiags := cfg.collectInputVariableValues(tt.args.env, files, tt.args.argv) + if (gotDiags == nil) == tt.wantDiags { t.Fatalf("Variables.collectVariableValues() = %v, want %v", gotDiags, tt.wantDiags) } + if tt.wantDiagsHasError != gotDiags.HasErrors() { + t.Fatalf("Variables.collectVariableValues() unexpected diagnostics HasErrors. %s", gotDiags) + } if diff := cmp.Diff(fmt.Sprintf("%#v", tt.wantVariables), fmt.Sprintf("%#v", tt.variables)); diff != "" { t.Fatalf("didn't get expected variables: %s", diff) }