Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HPR-837] Update validation options for undeclared variables #12104

Merged
merged 3 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion command/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 2 additions & 10 deletions command/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
17 changes: 12 additions & 5 deletions command/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -129,14 +135,15 @@ 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)
}

// 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) {
Expand Down
3 changes: 3 additions & 0 deletions command/meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 17 additions & 0 deletions command/test-fixtures/validate/var-file-tests/basic.pkr.hcl
Original file line number Diff line number Diff line change
@@ -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"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test = "myvalue"
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"unused": ["one", "cookie", "two", "cookie"]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
unused = "unused variables should warn"
19 changes: 13 additions & 6 deletions command/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
129 changes: 129 additions & 0 deletions command/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
7 changes: 5 additions & 2 deletions hcl2template/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,11 @@ type Parser struct {

CorePackerVersionString string

*hclparse.Parser

PluginConfig *packer.PluginConfig

ValidationOptions

*hclparse.Parser
}

const (
Expand Down Expand Up @@ -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,
}
Expand Down
2 changes: 1 addition & 1 deletion hcl2template/types.packer_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ type PackerConfig struct {
}

type ValidationOptions struct {
Strict bool
WarnOnUndeclaredVar bool
}

const (
Expand Down
18 changes: 9 additions & 9 deletions hcl2template/types.variables.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More a question than anything, is null a valid default value for a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this does work. We could set the default value to an empty string but that may not immediately imply to the user that the variable is optional. We have the use of default = null documented as the preferred way to make a value of any type optional. So I went with that.

I've considered adding a comment to the example block to indicate that both type and the default value should be changed to reflect the actual value in the var-file. But that might be too much info. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think you're right, it may be a bit much, just stating they need to declare a variable block to make their template valid is good enough imo, that was just me asking a question out of curiosity, I didn't know that null was accepted for any type and was the preferred way to make a variable optional.
If that's documented and known, no issue in proposing null to be the default value, users can choose whatever else suits their need should they want to.
Besides it's just an example of what they need to add to their template, they'll have to change its contents 90% of the time because the default value is not required, different, or because the type is not string, so I don't think we need to worry too much about what's in there.

"}",
name),
Context: attr.Range.Ptr(),
Expand Down
8 changes: 4 additions & 4 deletions hcl2template/types.variables_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -850,7 +850,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
},

// output
wantDiags: true,
wantDiags: false,
wantDiagsHasError: false,
wantVariables: Variables{},
wantValues: map[string]cty.Value{},
Expand All @@ -859,15 +859,15 @@ 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"`},
},

// output
wantDiags: true,
wantDiagsHasError: true,
wantDiagsHasError: false,
wantVariables: Variables{},
wantValues: map[string]cty.Value{},
},
Expand Down