refactor initialization out from packer configs + tests (#9627)

The initialization of packer core in JSON also validates that `null` variables were set, except in the case of `packer validate --syntax-only` , but after the refactor to allow to have all commands work with HCL2 and JSON this subtlety was lost.

This refactors the initialisation of the core in order to allow to have `packer validate --syntax-only` not error in case a variable is not set. Since these calls are refactored this works for HCL2 too.

fix #9478

builder/vmware/iso/step_create_vmx_test.go

@@ -156,7 +156,8 @@ func setupVMwareBuild(t *testing.T, builderConfig map[string]string, provisioner
 	}
 
 	// create a core using our template
-	core, err := packer.NewCore(&config)
+	core := packer.NewCore(&config)
+	err = core.Initialize()
 	if err != nil {
 		t.Fatalf("Unable to create core: %s", err)
 	}

command/build.go

@@ -109,7 +109,7 @@ func (m *Meta) GetConfig(cla *MetaArgs) (packer.Handler, int) {
 	}
 }
 
-func (m *Meta) GetConfigFromJSON(cla *MetaArgs) (*packer.Core, int) {
+func (m *Meta) GetConfigFromJSON(cla *MetaArgs) (packer.Handler, int) {
 	// Parse the template
 	var tpl *template.Template
 	var err error
@@ -133,7 +133,7 @@ func (m *Meta) GetConfigFromJSON(cla *MetaArgs) (*packer.Core, int) {
 		m.Ui.Error(err.Error())
 		ret = 1
 	}
-	return core, ret
+	return &CoreWrapper{core}, ret
 }
 
 func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int {
@@ -141,6 +141,11 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int
 	if ret != 0 {
 		return ret
 	}
+	diags := packerStarter.Initialize()
+	ret = writeDiags(c.Ui, nil, diags)
+	if ret != 0 {
+		return ret
+	}
 
 	builds, diags := packerStarter.GetBuilds(packer.GetBuildsOptions{
 		Only:    cla.Only,

command/console.go

@@ -60,6 +60,12 @@ func (c *ConsoleCommand) RunContext(ctx context.Context, cla *ConsoleArgs) int {
 		return ret
 	}
 
+	diags := packerStarter.Initialize()
+	ret = writeDiags(c.Ui, nil, diags)
+	if ret != 0 {
+		return ret
+	}
+
 	// Determine if stdin is a pipe. If so, we evaluate directly.
 	if c.StdinPiped() {
 		return c.modePiped(packerStarter)

command/core_wrapper.go

@@ -0,0 +1,25 @@
+package command
+
+import (
+	"github.com/hashicorp/hcl/v2"
+	"github.com/hashicorp/packer/packer"
+)
+
+// CoreWrapper wraps a packer.Core in order to have it's Initialize func return
+// a diagnostic.
+type CoreWrapper struct {
+	*packer.Core
+}
+
+func (c *CoreWrapper) Initialize() hcl.Diagnostics {
+	err := c.Core.Initialize()
+	if err != nil {
+		return hcl.Diagnostics{
+			&hcl.Diagnostic{
+				Detail:   err.Error(),
+				Severity: hcl.DiagError,
+			},
+		}
+	}
+	return nil
+}

command/inspect.go

@@ -44,6 +44,11 @@ func (c *InspectCommand) RunContext(ctx context.Context, cla *InspectArgs) int {
 	if ret != 0 {
 		return ret
 	}
+	diags := packerStarter.Initialize()
+	ret = writeDiags(c.Ui, nil, diags)
+	if ret != 0 {
+		return ret
+	}
 	return packerStarter.InspectConfig(packer.InspectConfigOptions{
 		Ui: c.Ui,
 	})

command/meta.go

@@ -3,7 +3,6 @@ package command
 import (
 	"bufio"
 	"flag"
-	"fmt"
 	"io"
 	"os"
 
@@ -59,12 +58,7 @@ func (m *Meta) Core(tpl *template.Template, cla *MetaArgs) (*packer.Core, error)
 	}
 	config.Variables = cla.Vars
 
-	// Init the core
-	core, err := packer.NewCore(&config)
-	if err != nil {
-		return nil, fmt.Errorf("Error initializing core: %s", err)
-	}
-
+	core := packer.NewCore(&config)
 	return core, nil
 }
 

command/test-fixtures/validate/null_var.json

@@ -0,0 +1,15 @@
+{
+    "variables": {
+        "null_var": null
+    },
+    "builders": [{
+        "type": "null",
+        "communicator": "none"
+    }],
+    "provisioners": [
+        {
+            "type": "shell-local",
+            "inline": "echo yop"
+        }
+    ]
+}

command/test-fixtures/validate/var_foo_with_no_default.pkr.hcl

@@ -0,0 +1,3 @@
+
+variable "foo" {
+}

command/validate.go

@@ -52,10 +52,17 @@ func (c *ValidateCommand) RunContext(ctx context.Context, cla *ValidateArgs) int
 
 	// If we're only checking syntax, then we're done already
 	if cla.SyntaxOnly {
+		c.Ui.Say("Syntax-only check passed. Everything looks okay.")
 		return 0
 	}
 
-	_, diags := packerStarter.GetBuilds(packer.GetBuildsOptions{
+	diags := packerStarter.Initialize()
+	ret = writeDiags(c.Ui, nil, diags)
+	if ret != 0 {
+		return ret
+	}
+
+	_, diags = packerStarter.GetBuilds(packer.GetBuildsOptions{
 		Only:   cla.Only,
 		Except: cla.Except,
 	})

command/validate_test.go

@@ -3,6 +3,8 @@ package command
 import (
 	"path/filepath"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestValidateCommand(t *testing.T) {
@@ -15,14 +17,15 @@ func TestValidateCommand(t *testing.T) {
 		{path: filepath.Join(testFixture("validate"), "build_with_vars.pkr.hcl")},
 		{path: filepath.Join(testFixture("validate-invalid"), "bad_provisioner.json"), exitCode: 1},
 		{path: filepath.Join(testFixture("validate-invalid"), "missing_build_block.pkr.hcl"), exitCode: 1},
-	}
-
-	c := &ValidateCommand{
-		Meta: testMetaFile(t),
+		{path: filepath.Join(testFixture("validate"), "null_var.json"), exitCode: 1},
+		{path: filepath.Join(testFixture("validate"), "var_foo_with_no_default.pkr.hcl"), exitCode: 1},
 	}
 
 	for _, tc := range tt {
 		t.Run(tc.path, func(t *testing.T) {
+			c := &ValidateCommand{
+				Meta: testMetaFile(t),
+			}
 			tc := tc
 			args := []string{tc.path}
 			if code := c.Run(args); code != tc.exitCode {
@@ -43,15 +46,16 @@ func TestValidateCommand_SyntaxOnly(t *testing.T) {
 		{path: filepath.Join(testFixture("validate-invalid"), "bad_provisioner.json")},
 		{path: filepath.Join(testFixture("validate-invalid"), "missing_build_block.pkr.hcl")},
 		{path: filepath.Join(testFixture("validate-invalid"), "broken.json"), exitCode: 1},
+		{path: filepath.Join(testFixture("validate"), "null_var.json")},
+		{path: filepath.Join(testFixture("validate"), "var_foo_with_no_default.pkr.hcl")},
 	}
 
+	for _, tc := range tt {
+		t.Run(tc.path, func(t *testing.T) {
 			c := &ValidateCommand{
 				Meta: testMetaFile(t),
 			}
 			c.CoreConfig.Version = "102.0.0"
-
-	for _, tc := range tt {
-		t.Run(tc.path, func(t *testing.T) {
 			tc := tc
 			args := []string{"-syntax-only", tc.path}
 			if code := c.Run(args); code != tc.exitCode {
@@ -91,10 +95,15 @@ func TestValidateCommandBadVersion(t *testing.T) {
 	}
 
 	stdout, stderr := outputCommand(t, c.Meta)
-	expected := `Error initializing core: This template requires Packer version 101.0.0 or higher; using 100.0.0
+	expected := `Error: 
+
+This template requires Packer version 101.0.0 or higher; using 100.0.0
+
+
 `
-	if stderr != expected {
-		t.Fatalf("Expected:\n%s\nFound:\n%s\n", expected, stderr)
+
+	if diff := cmp.Diff(expected, stderr); diff != "" {
+		t.Errorf("Unexpected output: %s", diff)
 	}
 	t.Log(stdout)
 }

hcl2template/common_test.go

@@ -61,6 +61,8 @@ func testParse(t *testing.T, tests []parseTest) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			gotCfg, gotDiags := tt.parser.Parse(tt.args.filename, tt.args.varFiles, tt.args.vars)
+			moreDiags := gotCfg.Initialize()
+			gotDiags = append(gotDiags, moreDiags...)
 			if tt.parseWantDiags == (gotDiags == nil) {
 				t.Fatalf("Parser.parse() unexpected %q diagnostics.", gotDiags)
 			}
@@ -81,6 +83,7 @@ func testParse(t *testing.T, tests []parseTest) {
 					"Cwd", // Cwd will change for every computer
 				),
 				cmpopts.IgnoreTypes(HCL2Ref{}),
+				cmpopts.IgnoreTypes([]*LocalBlock{}),
 				cmpopts.IgnoreTypes([]hcl.Range{}),
 				cmpopts.IgnoreTypes(hcl.Range{}),
 				cmpopts.IgnoreInterfaces(struct{ hcl.Expression }{}),

hcl2template/parser.go

@@ -110,11 +110,12 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 		builderSchemas:        p.BuilderSchemas,
 		provisionersSchemas:   p.ProvisionersSchemas,
 		postProcessorsSchemas: p.PostProcessorsSchemas,
+		parser:                p,
+		files:                 files,
 	}
 
 	// Decode variable blocks so that they are available later on. Here locals
 	// can use input variables so we decode them firsthand.
-	var locals []*Local
 	{
 		for _, file := range files {
 			diags = append(diags, cfg.decodeInputVariables(file)...)
@@ -123,7 +124,7 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 		for _, file := range files {
 			moreLocals, morediags := cfg.parseLocalVariables(file)
 			diags = append(diags, morediags...)
-			locals = append(locals, moreLocals...)
+			cfg.LocalBlocks = append(cfg.LocalBlocks, moreLocals...)
 		}
 	}
 
@@ -165,19 +166,24 @@ func (p *Parser) Parse(filename string, varFiles []string, argVars map[string]st
 
 		diags = append(diags, cfg.collectInputVariableValues(os.Environ(), varFiles, argVars)...)
 	}
+	return cfg, diags
+}
+
+func (cfg *PackerConfig) Initialize() hcl.Diagnostics {
+	var diags hcl.Diagnostics
 
 	_, moreDiags := cfg.InputVariables.Values()
 	diags = append(diags, moreDiags...)
 	_, moreDiags = cfg.LocalVariables.Values()
 	diags = append(diags, moreDiags...)
-	diags = append(diags, cfg.evaluateLocalVariables(locals)...)
+	diags = append(diags, cfg.evaluateLocalVariables(cfg.LocalBlocks)...)
 
 	// decode the actual content
-	for _, file := range files {
-		diags = append(diags, p.decodeConfig(file, cfg)...)
+	for _, file := range cfg.files {
+		diags = append(diags, cfg.parser.decodeConfig(file, cfg)...)
 	}
 
-	return cfg, diags
+	return diags
 }
 
 // decodeConfig looks in the found blocks for everything that is not a variable

hcl2template/types.packer_config.go

@@ -29,6 +29,8 @@ type PackerConfig struct {
 	InputVariables Variables
 	LocalVariables Variables
 
+	LocalBlocks []*LocalBlock
+
 	ValidationOptions
 
 	// Builds is the list of Build blocks defined in the config files.
@@ -42,6 +44,9 @@ type PackerConfig struct {
 
 	except []glob.Glob
 	only   []glob.Glob
+
+	parser *Parser
+	files  []*hcl.File
 }
 
 type ValidationOptions struct {
@@ -113,12 +118,12 @@ func (c *PackerConfig) decodeInputVariables(f *hcl.File) hcl.Diagnostics {
 // parseLocalVariables looks in the found blocks for 'locals' blocks. It
 // should be called after parsing input variables so that they can be
 // referenced.
-func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*Local, hcl.Diagnostics) {
+func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*LocalBlock, hcl.Diagnostics) {
 	var diags hcl.Diagnostics
 
 	content, moreDiags := f.Body.Content(configSchema)
 	diags = append(diags, moreDiags...)
-	var locals []*Local
+	var locals []*LocalBlock
 
 	for _, block := range content.Blocks {
 		switch block.Type {
@@ -136,7 +141,7 @@ func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*Local, hcl.Diagnosti
 					})
 					return nil, diags
 				}
-				locals = append(locals, &Local{
+				locals = append(locals, &LocalBlock{
 					Name: name,
 					Expr: attr.Expr,
 				})
@@ -147,7 +152,7 @@ func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*Local, hcl.Diagnosti
 	return locals, diags
 }
 
-func (c *PackerConfig) evaluateLocalVariables(locals []*Local) hcl.Diagnostics {
+func (c *PackerConfig) evaluateLocalVariables(locals []*LocalBlock) hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
 	if len(locals) > 0 && c.LocalVariables == nil {
@@ -187,7 +192,7 @@ func (c *PackerConfig) evaluateLocalVariables(locals []*Local) hcl.Diagnostics {
 	return diags
 }
 
-func (c *PackerConfig) evaluateLocalVariable(local *Local) hcl.Diagnostics {
+func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
 	value, moreDiags := local.Expr.Value(c.EvalContext(nil))

hcl2template/types.variables.go

@@ -15,7 +15,7 @@ import (
 // Local represents a single entry from a "locals" block in a module or file.
 // The "locals" block itself is not represented, because it serves only to
 // provide context for us to interpret its contents.
-type Local struct {
+type LocalBlock struct {
 	Name string
 	Expr hcl.Expression
 }

helper/builder/testing/testing.go

@@ -111,7 +111,7 @@ func Test(t TestT, c TestCase) {
 
 	// Build the core
 	log.Printf("[DEBUG] Initializing core...")
-	core, err := packer.NewCore(&packer.CoreConfig{
+	core := packer.NewCore(&packer.CoreConfig{
 		Components: packer.ComponentFinder{
 			BuilderStore: TestBuilderStore{
 				StartFn: func(n string) (packer.Builder, error) {
@@ -125,6 +125,7 @@ func Test(t TestT, c TestCase) {
 		},
 		Template: tpl,
 	})
+	err = core.Initialize()
 	if err != nil {
 		t.Fatal(fmt.Sprintf("Failed to init core: %s", err))
 		return

packer/core.go

@@ -92,8 +92,8 @@ type ComponentFinder struct {
 }
 
 // NewCore creates a new Core.
-func NewCore(c *CoreConfig) (*Core, error) {
-	result := &Core{
+func NewCore(c *CoreConfig) *Core {
+	core := &Core{
 		Template:   c.Template,
 		components: c.Components,
 		variables:  c.Variables,
@@ -101,31 +101,34 @@ func NewCore(c *CoreConfig) (*Core, error) {
 		only:       c.Only,
 		except:     c.Except,
 	}
+	return core
+}
 
-	if err := result.validate(); err != nil {
-		return nil, err
+func (core *Core) Initialize() error {
+	if err := core.validate(); err != nil {
+		return err
 	}
-	if err := result.init(); err != nil {
-		return nil, err
+	if err := core.init(); err != nil {
+		return err
 	}
-	for _, secret := range result.secrets {
+	for _, secret := range core.secrets {
 		LogSecretFilter.Set(secret)
 	}
 
 	// Go through and interpolate all the build names. We should be able
 	// to do this at this point with the variables.
-	result.builds = make(map[string]*template.Builder)
-	for _, b := range c.Template.Builders {
-		v, err := interpolate.Render(b.Name, result.Context())
+	core.builds = make(map[string]*template.Builder)
+	for _, b := range core.Template.Builders {
+		v, err := interpolate.Render(b.Name, core.Context())
 		if err != nil {
-			return nil, fmt.Errorf(
+			return fmt.Errorf(
 				"Error interpolating builder '%s': %s",
 				b.Name, err)
 		}
 
-		result.builds[v] = b
+		core.builds[v] = b
 	}
-	return result, nil
+	return nil
 }
 
 // BuildNames returns the builds that are available in this configured core.

packer/core_test.go

@@ -37,10 +37,11 @@ func TestCoreBuildNames(t *testing.T) {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
 
-		core, err := NewCore(&CoreConfig{
+		core := NewCore(&CoreConfig{
 			Template:  tpl,
 			Variables: tc.Vars,
 		})
+		err = core.Initialize()
 		if err != nil {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
@@ -487,11 +488,12 @@ func TestCoreValidate(t *testing.T) {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
 
-		_, err = NewCore(&CoreConfig{
+		core := NewCore(&CoreConfig{
 			Template:  tpl,
 			Variables: tc.Vars,
 			Version:   "1.0.0",
 		})
+		err = core.Initialize()
 
 		if (err != nil) != tc.Err {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
@@ -535,10 +537,11 @@ func TestCore_InterpolateUserVars(t *testing.T) {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
 
-		ccf, err := NewCore(&CoreConfig{
+		ccf := NewCore(&CoreConfig{
 			Template: tpl,
 			Version:  "1.0.0",
 		})
+		err = ccf.Initialize()
 
 		if (err != nil) != tc.Err {
 			if tc.Err == false {
@@ -606,11 +609,12 @@ func TestCore_InterpolateUserVars_VarFile(t *testing.T) {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
 
-		ccf, err := NewCore(&CoreConfig{
+		ccf := NewCore(&CoreConfig{
 			Template:  tpl,
 			Version:   "1.0.0",
 			Variables: tc.Variables,
 		})
+		err = ccf.Initialize()
 
 		if (err != nil) != tc.Err {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
@@ -665,11 +669,12 @@ func TestSensitiveVars(t *testing.T) {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
 		}
 
-		_, err = NewCore(&CoreConfig{
+		ccf := NewCore(&CoreConfig{
 			Template:  tpl,
 			Variables: tc.Vars,
 			Version:   "1.0.0",
 		})
+		err = ccf.Initialize()
 
 		if (err != nil) != tc.Err {
 			t.Fatalf("err: %s\n\n%s", tc.File, err)
@@ -744,7 +749,7 @@ func TestEnvAndFileVars(t *testing.T) {
 		t.Fatalf("err: %s\n\n%s", "complex-recursed-env-user-var-file.json", err)
 	}
 
-	ccf, err := NewCore(&CoreConfig{
+	ccf := NewCore(&CoreConfig{
 		Template: tpl,
 		Version:  "1.0.0",
 		Variables: map[string]string{
@@ -753,6 +758,8 @@ func TestEnvAndFileVars(t *testing.T) {
 			"final_var": "{{user `env_1`}}/{{user `env_2`}}/{{user `env_4`}}{{user `env_3`}}-{{user `var_1`}}/vmware/{{user `var_2`}}.vmx",
 		},
 	})
+	err = ccf.Initialize()
+
 	expected := map[string]string{
 		"var_1":     "partyparrot",
 		"var_2":     "bulbasaur-5/path/to/nowhere-partyparrot",

packer/run_interfaces.go

@@ -26,6 +26,7 @@ type Evaluator interface {
 
 // The packer.Handler handles all Packer things.
 type Handler interface {
+	Initialize() hcl.Diagnostics
 	Evaluator
 	BuildGetter
 	ConfigFixer

packer/testing.go

@@ -20,7 +20,8 @@ func TestCoreConfig(t *testing.T) *CoreConfig {
 }
 
 func TestCore(t *testing.T, c *CoreConfig) *Core {
-	core, err := NewCore(c)
+	core := NewCore(c)
+	err := core.Initialize()
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}