From 44616d3bffaddfd31c7e9fa6c2a321172e028d20 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 24 Jul 2020 10:58:03 +0200 Subject: [PATCH] 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 | 3 +- command/build.go | 9 +++-- command/console.go | 6 ++++ command/core_wrapper.go | 25 ++++++++++++++ command/inspect.go | 5 +++ command/meta.go | 8 +---- command/test-fixtures/validate/null_var.json | 15 +++++++++ .../validate/var_foo_with_no_default.pkr.hcl | 3 ++ command/validate.go | 9 ++++- command/validate_test.go | 33 ++++++++++++------- hcl2template/common_test.go | 3 ++ hcl2template/parser.go | 18 ++++++---- hcl2template/types.packer_config.go | 15 ++++++--- hcl2template/types.variables.go | 2 +- helper/builder/testing/testing.go | 3 +- packer/core.go | 29 ++++++++-------- packer/core_test.go | 19 +++++++---- packer/run_interfaces.go | 1 + packer/testing.go | 3 +- 19 files changed, 153 insertions(+), 56 deletions(-) create mode 100644 command/core_wrapper.go create mode 100644 command/test-fixtures/validate/null_var.json create mode 100644 command/test-fixtures/validate/var_foo_with_no_default.pkr.hcl diff --git a/builder/vmware/iso/step_create_vmx_test.go b/builder/vmware/iso/step_create_vmx_test.go index 2ecaefe8c..480afccc3 100644 --- a/builder/vmware/iso/step_create_vmx_test.go +++ b/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) } diff --git a/command/build.go b/command/build.go index d463489c9..8df375e28 100644 --- a/command/build.go +++ b/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, diff --git a/command/console.go b/command/console.go index 2bdec1042..ea43771b2 100644 --- a/command/console.go +++ b/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) diff --git a/command/core_wrapper.go b/command/core_wrapper.go new file mode 100644 index 000000000..92065d285 --- /dev/null +++ b/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 +} diff --git a/command/inspect.go b/command/inspect.go index ddfc6fd9a..d304c7926 100644 --- a/command/inspect.go +++ b/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, }) diff --git a/command/meta.go b/command/meta.go index 6f37f14c4..910ffb16f 100644 --- a/command/meta.go +++ b/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 } diff --git a/command/test-fixtures/validate/null_var.json b/command/test-fixtures/validate/null_var.json new file mode 100644 index 000000000..016e20b19 --- /dev/null +++ b/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" + } + ] +} diff --git a/command/test-fixtures/validate/var_foo_with_no_default.pkr.hcl b/command/test-fixtures/validate/var_foo_with_no_default.pkr.hcl new file mode 100644 index 000000000..57d1e68a1 --- /dev/null +++ b/command/test-fixtures/validate/var_foo_with_no_default.pkr.hcl @@ -0,0 +1,3 @@ + +variable "foo" { +} \ No newline at end of file diff --git a/command/validate.go b/command/validate.go index 65912717e..8fa60df03 100644 --- a/command/validate.go +++ b/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, }) diff --git a/command/validate_test.go b/command/validate_test.go index 5dfbd97a9..9ed2a1991 100644 --- a/command/validate_test.go +++ b/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")}, } - c := &ValidateCommand{ - Meta: testMetaFile(t), - } - c.CoreConfig.Version = "102.0.0" - for _, tc := range tt { t.Run(tc.path, func(t *testing.T) { + c := &ValidateCommand{ + Meta: testMetaFile(t), + } + c.CoreConfig.Version = "102.0.0" 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) } diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index 3436b6d86..48eec28fa 100644 --- a/hcl2template/common_test.go +++ b/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 }{}), diff --git a/hcl2template/parser.go b/hcl2template/parser.go index c2544c72b..c14dd3a6b 100644 --- a/hcl2template/parser.go +++ b/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 diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 946f453dd..52a7c550e 100644 --- a/hcl2template/types.packer_config.go +++ b/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)) diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index e97266d6f..6a2649083 100644 --- a/hcl2template/types.variables.go +++ b/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 } diff --git a/helper/builder/testing/testing.go b/helper/builder/testing/testing.go index df1bdad87..06b0a31b6 100644 --- a/helper/builder/testing/testing.go +++ b/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 diff --git a/packer/core.go b/packer/core.go index 89ff7068e..23abdc983 100644 --- a/packer/core.go +++ b/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. diff --git a/packer/core_test.go b/packer/core_test.go index 1f9cfb816..1a4f865df 100644 --- a/packer/core_test.go +++ b/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", diff --git a/packer/run_interfaces.go b/packer/run_interfaces.go index 31a0dedcd..7e428fbe7 100644 --- a/packer/run_interfaces.go +++ b/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 diff --git a/packer/testing.go b/packer/testing.go index 000c57e70..4df1b5de7 100644 --- a/packer/testing.go +++ b/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) }