From 57cbe4e203bdfcfe5658898b3c502ed5d9ee3f65 Mon Sep 17 00:00:00 2001 From: Wilken Rivera Date: Mon, 14 Nov 2022 17:06:45 -0500 Subject: [PATCH] core: Update validation options for undeclared variables (#12104) * Update validation options for undeclared variables In an effort to help users move from JSON to HCL2 templates the support for variable definitions files are being updated to ignore undeclared variable warnings on build execution. For legacy JSON templates builds no warnings are displayed when var-files contain undeclared variables. Since preferred mode HCL2 templates is to be explicit with variable declarations - they must be declared to be used - validation for undeclared variables still warns when running `packer validate`. A new flag has been added to the validate command that can be used to disable undeclared variable warnings. * Update validation test for unused variables Example Run ``` ~> go run . validate -no-warn-undeclared-var -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl The configuration is valid. ~> go run . validate -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl Warning: Undefined variable The variable "unused" was set but was not declared as an input variable. To declare variable "unused" place this block in one of your .pkr.hcl files, such as variables.pkr.hcl variable "unused" { type = string default = null } The configuration is valid. ~> go run . build -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl file.chocolate: output will be in this color. Build 'file.chocolate' finished after 744 microseconds. ==> Wait completed after 798 microseconds ==> Builds finished. The artifacts of successful builds are: --> file.chocolate: Stored file: chocolate.txt ``` * Rename Strict field to WarnOnUndeclaredVar The field name Strict is a bit vague since it is only used for checking against undeclared variables within a var-file definition. To mitigate against potential overloading of this field it is being renamed to be more explicit on its usage. * command/build: Add warn-on-undeclared-var flag Now that the default behaviour is to not display warnings for undeclared variables an optional flag has been added to toggle the old behaviour. ``` ~> go run . build -warn-on-undeclared-var -var-file command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl command/test-fixtures/validate/var-file-tests/basic.pkr.hcl Warning: Undefined variable The variable "unused" was set but was not declared as an input variable. To declare variable "unused" place this block in one of your .pkr.hcl files, such as variables.pkr.hcl variable "unused" { type = string default = null } file.chocolate: output will be in this color. Build 'file.chocolate' finished after 762 microseconds. ==> Wait completed after 799 microseconds ==> Builds finished. The artifacts of successful builds are: --> file.chocolate: Stored file: chocolate.txt ``` --- command/build.go | 3 +- command/build_test.go | 12 +- command/cli.go | 17 ++- command/meta.go | 3 + .../validate/var-file-tests/basic.pkr.hcl | 17 +++ .../validate/var-file-tests/basic.pkrvars.hcl | 1 + .../validate/var-file-tests/undeclared.json | 3 + .../var-file-tests/undeclared.pkrvars.hcl | 1 + command/validate.go | 19 ++- command/validate_test.go | 129 ++++++++++++++++++ hcl2template/parser.go | 7 +- hcl2template/types.packer_config.go | 2 +- hcl2template/types.variables.go | 18 +-- hcl2template/types.variables_test.go | 8 +- 14 files changed, 202 insertions(+), 38 deletions(-) create mode 100644 command/test-fixtures/validate/var-file-tests/basic.pkr.hcl create mode 100644 command/test-fixtures/validate/var-file-tests/basic.pkrvars.hcl create mode 100644 command/test-fixtures/validate/var-file-tests/undeclared.json create mode 100644 command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl diff --git a/command/build.go b/command/build.go index d2977d0ae36..1cf9cdd61cc 100644 --- a/command/build.go +++ b/command/build.go @@ -400,7 +400,8 @@ Options: -parallel-builds=1 Number of builds to run in parallel. 1 disables parallelization. 0 means no limit (Default: 0) -timestamp-ui Enable prefixing of each ui output with an RFC3339 timestamp. -var 'key=value' Variable for templates, can be used multiple times. - -var-file=path JSON or HCL2 file containing user variables. + -var-file=path JSON or HCL2 file containing user variables, can be used multiple times. + -warn-on-undeclared-var Display warnings for user variable files containing undeclared variables. ` return strings.TrimSpace(helpText) diff --git a/command/build_test.go b/command/build_test.go index e354fe02a97..a306fbde252 100644 --- a/command/build_test.go +++ b/command/build_test.go @@ -1149,21 +1149,13 @@ func TestBuildCmd(t *testing.T) { }, expectedCode: 0, outputCheck: func(out, err string) error { - if !strings.Contains(out, "Warning: Undefined variable") { - return fmt.Errorf("expected 'Warning: Undefined variable' in output, did not find it") - } - nbWarns := strings.Count(out, "Warning: ") - if nbWarns != 1 { + if nbWarns != 0 { return fmt.Errorf( - "error: too many warnings in build output, expected 1, got %d", + "error: too many warnings in build output, expected 0, got %d", nbWarns) } - if !strings.Contains(out, "variable \"testvar\" {") { - return fmt.Errorf("missing definition example for undefined variable") - } - nbErrs := strings.Count(err, "Error: ") if nbErrs != 0 { return fmt.Errorf("error: expected build to succeed without errors, got %d", diff --git a/command/cli.go b/command/cli.go index a9486cd5282..826835ef2b3 100644 --- a/command/cli.go +++ b/command/cli.go @@ -58,7 +58,7 @@ func (ma *MetaArgs) AddFlagSets(fs *flag.FlagSet) { fs.Var(&ma.ConfigType, "config-type", "set to 'hcl2' to run in hcl2 mode when no file is passed.") } -// MetaArgs defines commonalities between all comands +// MetaArgs defines commonalities between all commands type MetaArgs struct { // TODO(azr): in the future, I want to allow passing multiple path to // merge HCL confs together; but this will probably need an RFC first. @@ -68,6 +68,10 @@ type MetaArgs struct { VarFiles []string // set to "hcl2" to force hcl2 mode ConfigType configType + + // WarnOnUndeclared does not have a common default, as the default varies per sub-command usage. + // Refer to individual command FlagSets for usage. + WarnOnUndeclaredVar bool } func (ba *BuildArgs) AddFlagSets(flags *flag.FlagSet) { @@ -82,15 +86,17 @@ func (ba *BuildArgs) AddFlagSets(flags *flag.FlagSet) { flagOnError := enumflag.New(&ba.OnError, "cleanup", "abort", "ask", "run-cleanup-provisioner") flags.Var(flagOnError, "on-error", "") + flags.BoolVar(&ba.MetaArgs.WarnOnUndeclaredVar, "warn-on-undeclared-var", false, "Show warnings for variable files containing undeclared variables.") ba.MetaArgs.AddFlagSets(flags) } // BuildArgs represents a parsed cli line for a `packer build` type BuildArgs struct { MetaArgs - Color, Debug, Force, TimestampUi, MachineReadable bool - ParallelBuilds int64 - OnError string + Debug, Force bool + Color, TimestampUi, MachineReadable bool + ParallelBuilds int64 + OnError string } func (ia *InitArgs) AddFlagSets(flags *flag.FlagSet) { @@ -129,6 +135,7 @@ type FixArgs struct { func (va *ValidateArgs) AddFlagSets(flags *flag.FlagSet) { flags.BoolVar(&va.SyntaxOnly, "syntax-only", false, "check syntax only") + flags.BoolVar(&va.NoWarnUndeclaredVar, "no-warn-undeclared-var", false, "Ignore warnings for variable files containing undeclared variables.") va.MetaArgs.AddFlagSets(flags) } @@ -136,7 +143,7 @@ func (va *ValidateArgs) AddFlagSets(flags *flag.FlagSet) { // ValidateArgs represents a parsed cli line for a `packer validate` type ValidateArgs struct { MetaArgs - SyntaxOnly bool + SyntaxOnly, NoWarnUndeclaredVar bool } func (va *InspectArgs) AddFlagSets(flags *flag.FlagSet) { diff --git a/command/meta.go b/command/meta.go index d6ad45ae96b..3f166595604 100644 --- a/command/meta.go +++ b/command/meta.go @@ -136,6 +136,9 @@ func (m *Meta) GetConfigFromHCL(cla *MetaArgs) (*hcl2template.PackerConfig, int) CorePackerVersionString: version.FormattedVersion(), Parser: hclparse.NewParser(), PluginConfig: m.CoreConfig.Components.PluginConfig, + ValidationOptions: hcl2template.ValidationOptions{ + WarnOnUndeclaredVar: cla.WarnOnUndeclaredVar, + }, } cfg, diags := parser.Parse(cla.Path, cla.VarFiles, cla.Vars) return cfg, writeDiags(m.Ui, parser.Files(), diags) diff --git a/command/test-fixtures/validate/var-file-tests/basic.pkr.hcl b/command/test-fixtures/validate/var-file-tests/basic.pkr.hcl new file mode 100644 index 00000000000..1fd332583be --- /dev/null +++ b/command/test-fixtures/validate/var-file-tests/basic.pkr.hcl @@ -0,0 +1,17 @@ +packer { + required_version = ">= v1.0.0" +} + +variable "test" { + type = string + default = null +} + +source "file" "chocolate" { + target = "chocolate.txt" + content = "chocolate" +} + +build { + sources = ["source.file.chocolate"] +} diff --git a/command/test-fixtures/validate/var-file-tests/basic.pkrvars.hcl b/command/test-fixtures/validate/var-file-tests/basic.pkrvars.hcl new file mode 100644 index 00000000000..efa60c7226d --- /dev/null +++ b/command/test-fixtures/validate/var-file-tests/basic.pkrvars.hcl @@ -0,0 +1 @@ +test = "myvalue" diff --git a/command/test-fixtures/validate/var-file-tests/undeclared.json b/command/test-fixtures/validate/var-file-tests/undeclared.json new file mode 100644 index 00000000000..5004198d96b --- /dev/null +++ b/command/test-fixtures/validate/var-file-tests/undeclared.json @@ -0,0 +1,3 @@ +{ + "unused": ["one", "cookie", "two", "cookie"] +} diff --git a/command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl b/command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl new file mode 100644 index 00000000000..dace598cee6 --- /dev/null +++ b/command/test-fixtures/validate/var-file-tests/undeclared.pkrvars.hcl @@ -0,0 +1 @@ +unused = "unused variables should warn" diff --git a/command/validate.go b/command/validate.go index c8ac937107a..304db1a380e 100644 --- a/command/validate.go +++ b/command/validate.go @@ -45,6 +45,12 @@ func (c *ValidateCommand) ParseArgs(args []string) (*ValidateArgs, int) { } func (c *ValidateCommand) RunContext(ctx context.Context, cla *ValidateArgs) int { + // By default we want to inform users of undeclared variables when validating but not during build time. + cla.MetaArgs.WarnOnUndeclaredVar = true + if cla.NoWarnUndeclaredVar { + cla.MetaArgs.WarnOnUndeclaredVar = false + } + packerStarter, ret := c.GetConfig(&cla.MetaArgs) if ret != 0 { return 1 @@ -95,12 +101,13 @@ Usage: packer validate [options] TEMPLATE Options: - -syntax-only Only check syntax. Do not verify config of the template. - -except=foo,bar,baz Validate all builds other than these. - -machine-readable Produce machine-readable output. - -only=foo,bar,baz Validate only these builds. - -var 'key=value' Variable for templates, can be used multiple times. - -var-file=path JSON or HCL2 file containing user variables. + -syntax-only Only check syntax. Do not verify config of the template. + -except=foo,bar,baz Validate all builds other than these. + -only=foo,bar,baz Validate only these builds. + -machine-readable Produce machine-readable output. + -var 'key=value' Variable for templates, can be used multiple times. + -var-file=path JSON or HCL2 file containing user variables, can be used multiple times. + -no-warn-undeclared-var Disable warnings for user variable files containing undeclared variables. ` return strings.TrimSpace(helpText) diff --git a/command/validate_test.go b/command/validate_test.go index eb63d73061d..5b9b346aaf6 100644 --- a/command/validate_test.go +++ b/command/validate_test.go @@ -205,3 +205,132 @@ func TestValidateCommandExcept(t *testing.T) { }) } } + +func TestValidateCommand_VarFiles(t *testing.T) { + tt := []struct { + name string + path string + varfile string + exitCode int + }{ + {name: "with basic HCL var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkrvars.hcl"), + exitCode: 0, + }, + {name: "with unused variable in var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.pkrvars.hcl"), + exitCode: 0, + }, + {name: "with unused variable in JSON var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.json"), + exitCode: 0, + }, + } + for _, tc := range tt { + t.Run(tc.path, func(t *testing.T) { + c := &ValidateCommand{ + Meta: TestMetaFile(t), + } + tc := tc + args := []string{"-var-file", tc.varfile, tc.path} + if code := c.Run(args); code != tc.exitCode { + fatalCommand(t, c.Meta) + } + }) + } +} + +func TestValidateCommand_VarFilesWarnOnUndeclared(t *testing.T) { + tt := []struct { + name string + path string + varfile string + exitCode int + }{ + {name: "default warning with unused variable in HCL var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.pkrvars.hcl"), + exitCode: 0, + }, + {name: "default warning with unused variable in JSON var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.json"), + exitCode: 0, + }, + } + for _, tc := range tt { + t.Run(tc.path, func(t *testing.T) { + c := &ValidateCommand{ + Meta: TestMetaFile(t), + } + tc := tc + args := []string{"-var-file", tc.varfile, tc.path} + if code := c.Run(args); code != tc.exitCode { + fatalCommand(t, c.Meta) + } + + stdout, stderr := GetStdoutAndErrFromTestMeta(t, c.Meta) + expected := `Warning: Undefined variable + +The variable "unused" was set but was not declared as an input variable. +To declare variable "unused" place this block in one of your .pkr.hcl files, +such as variables.pkr.hcl + +variable "unused" { + type = string + default = null +} + + +The configuration is valid. +` + if diff := cmp.Diff(expected, stdout); diff != "" { + t.Errorf("Unexpected output: %s", diff) + } + t.Log(stderr) + }) + } +} + +func TestValidateCommand_VarFilesDisableWarnOnUndeclared(t *testing.T) { + tt := []struct { + name string + path string + varfile string + exitCode int + }{ + {name: "no-warn-undeclared-var with unused variable in HCL var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.pkrvars.hcl"), + exitCode: 0, + }, + {name: "no-warn-undeclared-var with unused variable in JSON var-file definition", + path: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "basic.pkr.hcl"), + varfile: filepath.Join(testFixture(filepath.Join("validate", "var-file-tests")), "undeclared.json"), + exitCode: 0, + }, + } + for _, tc := range tt { + t.Run(tc.path, func(t *testing.T) { + c := &ValidateCommand{ + Meta: TestMetaFile(t), + } + tc := tc + args := []string{"-no-warn-undeclared-var", "-var-file", tc.varfile, tc.path} + if code := c.Run(args); code != tc.exitCode { + fatalCommand(t, c.Meta) + } + + stdout, stderr := GetStdoutAndErrFromTestMeta(t, c.Meta) + expected := `The configuration is valid. +` + if diff := cmp.Diff(expected, stdout); diff != "" { + t.Errorf("Unexpected output: %s", diff) + } + t.Log(stderr) + }) + } +} diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 95b18ad1b61..d2c5a85ecd2 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -60,9 +60,11 @@ type Parser struct { CorePackerVersionString string - *hclparse.Parser - PluginConfig *packer.PluginConfig + + ValidationOptions + + *hclparse.Parser } const ( @@ -138,6 +140,7 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st Cwd: wd, CorePackerVersionString: p.CorePackerVersionString, HCPVars: map[string]cty.Value{}, + ValidationOptions: p.ValidationOptions, parser: p, files: files, } diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 55e0f80ea50..8325d0e908b 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -68,7 +68,7 @@ type PackerConfig struct { } type ValidationOptions struct { - Strict bool + WarnOnUndeclaredVar bool } const ( diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 9131d5f3382..77928747c8d 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -659,19 +659,19 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F for name, attr := range attrs { variable, found := variables[name] if !found { - sev := hcl.DiagWarning - if cfg.ValidationOptions.Strict { - sev = hcl.DiagError + if !cfg.ValidationOptions.WarnOnUndeclaredVar { + continue } + diags = append(diags, &hcl.Diagnostic{ - Severity: sev, + Severity: hcl.DiagWarning, Summary: "Undefined variable", - Detail: fmt.Sprintf("A %[1]q variable was set but was "+ - "not declared as an input variable. To declare "+ - "variable %[1]q, place this block in one of your "+ - ".pkr.hcl files, such as variables.pkr.hcl\n\n"+ + Detail: fmt.Sprintf("The variable %[1]q was set but was not declared as an input variable."+ + "\nTo declare variable %[1]q place this block in one of your .pkr.hcl files, "+ + "such as variables.pkr.hcl\n\n"+ "variable %[1]q {\n"+ - " type = string\n"+ + " type = string\n"+ + " default = null\n"+ "}", name), Context: attr.Range.Ptr(), diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index 1abe331a85b..607862c9080 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -476,7 +476,7 @@ func TestParse_variables(t *testing.T) { }, Basedir: filepath.Join("testdata", "variables"), }, - true, false, + false, false, []packersdk.Build{ &packer.CoreBuild{ Type: "null.test", @@ -850,7 +850,7 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: false, wantDiagsHasError: false, wantVariables: Variables{}, wantValues: map[string]cty.Value{}, @@ -859,7 +859,7 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "undefined but set value - pkrvar file - strict mode", variables: Variables{}, validationOptions: ValidationOptions{ - Strict: true, + WarnOnUndeclaredVar: true, }, args: args{ hclFiles: []string{`undefined_string="value"`}, @@ -867,7 +867,7 @@ func TestVariables_collectVariableValues(t *testing.T) { // output wantDiags: true, - wantDiagsHasError: true, + wantDiagsHasError: false, wantVariables: Variables{}, wantValues: map[string]cty.Value{}, },