From c0e7e7bd3c02da7e13367afd9746ea8daadf9754 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 23 Sep 2022 16:59:17 -0400 Subject: [PATCH] hcl2: report error on build without sources When a template describes a build block without a source reference, the build should be considered invalid as we won't have a CoreBuild produced as a result of the need to have both. In current versions of Packer, this will produce an error message hinting that nothing will happen because of the lack of either build or source block. This commit takes the defined block, and points out to it as missing a source block as being the reason why nothing is happening, making it clearer what is required for an HCL2 build to be processed. --- command/build_test.go | 26 +++ .../test-fixtures/hcl/build_no_source.pkr.hcl | 1 + .../version_req/base_success/version.pkr.hcl | 8 +- hcl2template/common_test.go | 2 + .../build/post-processor_nonexistent.pkr.hcl | 3 + .../build/provisioner_nonexistent.pkr.hcl | 3 + .../testdata/datasources/basic.pkr.hcl | 8 +- .../testdata/datasources/recursive.pkr.hcl | 8 +- hcl2template/testdata/sources/basic.pkr.hcl | 8 +- hcl2template/testdata/variables/basic.pkr.hcl | 8 +- .../testdata/variables/complicated/d.pkr.hcl | 8 +- hcl2template/testdata/variables/empty.pkr.hcl | 8 +- .../variables/foo-string.variable.pkr.hcl | 8 +- .../variables/validation/valid.pkr.hcl | 8 +- hcl2template/types.build.go | 19 +++ hcl2template/types.build_test.go | 34 ++++ hcl2template/types.datasource_test.go | 62 +++++++- hcl2template/types.source_test.go | 38 ++++- hcl2template/types.variables_test.go | 150 ++++++++++++++++-- 19 files changed, 380 insertions(+), 30 deletions(-) create mode 100644 command/test-fixtures/hcl/build_no_source.pkr.hcl diff --git a/command/build_test.go b/command/build_test.go index d912e1319..e354fe02a 100644 --- a/command/build_test.go +++ b/command/build_test.go @@ -6,6 +6,7 @@ import ( "math" "os" "path/filepath" + "regexp" "runtime" "strings" "testing" @@ -1168,6 +1169,31 @@ func TestBuildCmd(t *testing.T) { return fmt.Errorf("error: expected build to succeed without errors, got %d", nbErrs) } + return nil + }, + }, + { + name: "hcl - build block without source", + args: []string{ + testFixture("hcl", "build_no_source.pkr.hcl"), + }, + expectedCode: 1, + outputCheck: func(_, err string) error { + if !strings.Contains(err, "Error: missing source reference") { + return fmt.Errorf("expected 'Error: missing source reference' in output, did not find it") + } + + nbErrs := strings.Count(err, "Error: ") + if nbErrs != 1 { + return fmt.Errorf( + "error: too many errors in stderr for build, expected 1, got %d", + nbErrs) + } + + logRegex := regexp.MustCompile("on.*build_no_source.pkr.hcl line 1") + if !logRegex.MatchString(err) { + return fmt.Errorf("error: missing context for error message") + } return nil }, diff --git a/command/test-fixtures/hcl/build_no_source.pkr.hcl b/command/test-fixtures/hcl/build_no_source.pkr.hcl new file mode 100644 index 000000000..af4fbda87 --- /dev/null +++ b/command/test-fixtures/hcl/build_no_source.pkr.hcl @@ -0,0 +1 @@ +build{} diff --git a/command/test-fixtures/version_req/base_success/version.pkr.hcl b/command/test-fixtures/version_req/base_success/version.pkr.hcl index d3ebe59b2..288698642 100644 --- a/command/test-fixtures/version_req/base_success/version.pkr.hcl +++ b/command/test-fixtures/version_req/base_success/version.pkr.hcl @@ -2,4 +2,10 @@ packer { required_version = ">= 1.0.0" } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index 619a3e19a..567959e21 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/packer/builder/null" dnull "github.com/hashicorp/packer/datasource/null" . "github.com/hashicorp/packer/hcl2template/internal" + hcl2template "github.com/hashicorp/packer/hcl2template/internal" packerregistry "github.com/hashicorp/packer/internal/registry" "github.com/hashicorp/packer/packer" "github.com/zclconf/go-cty/cty" @@ -372,6 +373,7 @@ var cmpOpts = []cmp.Option{ cmpopts.IgnoreFields(VariableAssignment{}, "Expr", // its an interface ), + cmpopts.IgnoreTypes(hcl2template.MockBuilder{}), cmpopts.IgnoreTypes(HCL2Ref{}), cmpopts.IgnoreTypes([]*LocalBlock{}), cmpopts.IgnoreTypes([]hcl.Range{}), diff --git a/hcl2template/testdata/build/post-processor_nonexistent.pkr.hcl b/hcl2template/testdata/build/post-processor_nonexistent.pkr.hcl index 3c1f20efe..4ea811f20 100644 --- a/hcl2template/testdata/build/post-processor_nonexistent.pkr.hcl +++ b/hcl2template/testdata/build/post-processor_nonexistent.pkr.hcl @@ -1,5 +1,8 @@ +source "null" "test" {} build { + sources = [ "null.test" ] + post-processor "nonexistent" { foo = "bar" } diff --git a/hcl2template/testdata/build/provisioner_nonexistent.pkr.hcl b/hcl2template/testdata/build/provisioner_nonexistent.pkr.hcl index d1f5b7c4e..a8d429057 100644 --- a/hcl2template/testdata/build/provisioner_nonexistent.pkr.hcl +++ b/hcl2template/testdata/build/provisioner_nonexistent.pkr.hcl @@ -1,5 +1,8 @@ +source "null" "test" {} build { + sources = [ "null.test" ] + provisioner "nonexistent" { foo = "bar" } diff --git a/hcl2template/testdata/datasources/basic.pkr.hcl b/hcl2template/testdata/datasources/basic.pkr.hcl index 4d7b9dcf9..70a51330a 100644 --- a/hcl2template/testdata/datasources/basic.pkr.hcl +++ b/hcl2template/testdata/datasources/basic.pkr.hcl @@ -86,4 +86,10 @@ data "amazon-ami" "test" { } } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/datasources/recursive.pkr.hcl b/hcl2template/testdata/datasources/recursive.pkr.hcl index 897395fbf..84acac92e 100644 --- a/hcl2template/testdata/datasources/recursive.pkr.hcl +++ b/hcl2template/testdata/datasources/recursive.pkr.hcl @@ -18,4 +18,10 @@ data "null" "bang" { input = "${data.null.baz.output}-with-marshmallows" } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/sources/basic.pkr.hcl b/hcl2template/testdata/sources/basic.pkr.hcl index c10d814dd..d7efb812e 100644 --- a/hcl2template/testdata/sources/basic.pkr.hcl +++ b/hcl2template/testdata/sources/basic.pkr.hcl @@ -87,4 +87,10 @@ source "virtualbox-iso" "ubuntu-1204" { } } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/variables/basic.pkr.hcl b/hcl2template/testdata/variables/basic.pkr.hcl index c0ef953f4..3c3c8132b 100644 --- a/hcl2template/testdata/variables/basic.pkr.hcl +++ b/hcl2template/testdata/variables/basic.pkr.hcl @@ -42,4 +42,10 @@ local "supersecret" { expression = "secretvar" } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/variables/complicated/d.pkr.hcl b/hcl2template/testdata/variables/complicated/d.pkr.hcl index d53f092e5..e005230b4 100644 --- a/hcl2template/testdata/variables/complicated/d.pkr.hcl +++ b/hcl2template/testdata/variables/complicated/d.pkr.hcl @@ -1 +1,7 @@ -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/variables/empty.pkr.hcl b/hcl2template/testdata/variables/empty.pkr.hcl index d53f092e5..e005230b4 100644 --- a/hcl2template/testdata/variables/empty.pkr.hcl +++ b/hcl2template/testdata/variables/empty.pkr.hcl @@ -1 +1,7 @@ -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/variables/foo-string.variable.pkr.hcl b/hcl2template/testdata/variables/foo-string.variable.pkr.hcl index adc6c3d04..f15ad1360 100644 --- a/hcl2template/testdata/variables/foo-string.variable.pkr.hcl +++ b/hcl2template/testdata/variables/foo-string.variable.pkr.hcl @@ -3,4 +3,10 @@ variable "foo" { default = "bar" } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/testdata/variables/validation/valid.pkr.hcl b/hcl2template/testdata/variables/validation/valid.pkr.hcl index 67c27dd26..dbab09d87 100644 --- a/hcl2template/testdata/variables/validation/valid.pkr.hcl +++ b/hcl2template/testdata/variables/validation/valid.pkr.hcl @@ -8,4 +8,10 @@ variable "image_id" { } } -build {} +source "null" "test" { + communicator = "none" +} + +build { + sources = ["null.test"] +} diff --git a/hcl2template/types.build.go b/hcl2template/types.build.go index dac9b5db4..3bb54b20c 100644 --- a/hcl2template/types.build.go +++ b/hcl2template/types.build.go @@ -110,7 +110,16 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block, cfg *PackerConfig) (*BuildB "name": cty.StringVal(b.Name), }) + // We rely on `hadSource` to determine which error to proc. + // + // If a source block is referenced in the build block, but isn't valid, we + // cannot rely on the `build.Sources' since it's only populated when a valid + // source is processed. + hadSource := false + for _, buildFrom := range b.FromSources { + hadSource = true + ref := sourceRefFromString(buildFrom) if ref == NoSource || @@ -156,6 +165,7 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block, cfg *PackerConfig) (*BuildB } build.HCPPackerRegistry = hcpPackerRegistry case sourceLabel: + hadSource = true ref, moreDiags := p.decodeBuildSource(block) diags = append(diags, moreDiags...) if moreDiags.HasErrors() { @@ -216,5 +226,14 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block, cfg *PackerConfig) (*BuildB } } + if !hadSource { + diags = append(diags, &hcl.Diagnostic{ + Summary: "missing source reference", + Detail: "a build block must reference at least one source to be built", + Severity: hcl.DiagError, + Subject: block.DefRange.Ptr(), + }) + } + return build, diags } diff --git a/hcl2template/types.build_test.go b/hcl2template/types.build_test.go index b5123a039..17cd80861 100644 --- a/hcl2template/types.build_test.go +++ b/hcl2template/types.build_test.go @@ -73,6 +73,15 @@ func TestParse_build(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Basedir: filepath.Join("testdata", "build"), + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, + }, Builds: Builds{ &BuildBlock{ ProvisionerBlocks: []*ProvisionerBlock{ @@ -80,6 +89,14 @@ func TestParse_build(t *testing.T) { PType: "nonexistent", }, }, + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, }, }, }, @@ -134,6 +151,15 @@ func TestParse_build(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Basedir: filepath.Join("testdata", "build"), + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, + }, Builds: Builds{ &BuildBlock{ PostProcessorsLists: [][]*PostProcessorBlock{ @@ -143,6 +169,14 @@ func TestParse_build(t *testing.T) { }, }, }, + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, }, }, }, diff --git a/hcl2template/types.datasource_test.go b/hcl2template/types.datasource_test.go index 078a8a2ba..26c8c3c45 100644 --- a/hcl2template/types.datasource_test.go +++ b/hcl2template/types.datasource_test.go @@ -5,6 +5,8 @@ import ( "testing" packersdk "github.com/hashicorp/packer-plugin-sdk/packer" + "github.com/hashicorp/packer/builder/null" + "github.com/hashicorp/packer/packer" ) func TestParse_datasource(t *testing.T) { @@ -17,7 +19,25 @@ func TestParse_datasource(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, Basedir: filepath.Join("testdata", "datasources"), Datasources: Datasources{ @@ -31,7 +51,15 @@ func TestParse_datasource(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, {"recursive datasources", @@ -40,7 +68,25 @@ func TestParse_datasource(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, Basedir: filepath.Join("testdata", "datasources"), Datasources: Datasources{ @@ -82,7 +128,15 @@ func TestParse_datasource(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, {"untyped datasource", diff --git a/hcl2template/types.source_test.go b/hcl2template/types.source_test.go index cc6d3fbb5..e19d921da 100644 --- a/hcl2template/types.source_test.go +++ b/hcl2template/types.source_test.go @@ -5,6 +5,8 @@ import ( "testing" packersdk "github.com/hashicorp/packer-plugin-sdk/packer" + "github.com/hashicorp/packer/builder/null" + "github.com/hashicorp/packer/packer" ) func TestParse_source(t *testing.T) { @@ -17,7 +19,16 @@ func TestParse_source(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, }, Basedir: filepath.Join("testdata", "sources"), Sources: map[SourceRef]SourceBlock{ @@ -28,10 +39,25 @@ func TestParse_source(t *testing.T) { Type: "virtualbox-iso", Name: "ubuntu-1204", }, + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, {"untyped source", @@ -61,15 +87,13 @@ func TestParse_source(t *testing.T) { parseTestArgs{"testdata/sources/nonexistent.pkr.hcl", nil, nil}, &PackerConfig{ CorePackerVersionString: lockedVersion, - Builds: Builds{ - &BuildBlock{}, - }, - Basedir: filepath.Join("testdata", "sources"), + Builds: nil, + Basedir: filepath.Join("testdata", "sources"), Sources: map[SourceRef]SourceBlock{ {Type: "nonexistent", Name: "ubuntu-1204"}: {Type: "nonexistent", Name: "ubuntu-1204"}, }, }, - false, false, + true, true, []packersdk.Build{}, false, }, diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index cbd4d63d4..1abe331a8 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -25,7 +25,25 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ @@ -105,7 +123,15 @@ func TestParse_variables(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, {"duplicate variable", @@ -279,7 +305,25 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, Basedir: "testdata/variables/complicated", InputVariables: Variables{ @@ -329,7 +373,15 @@ func TestParse_variables(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, {"recursive locals", @@ -352,7 +404,25 @@ func TestParse_variables(t *testing.T) { CorePackerVersionString: lockedVersion, Basedir: filepath.Join("testdata", "variables"), Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, InputVariables: Variables{ "foo": &Variable{ @@ -366,7 +436,15 @@ func TestParse_variables(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, @@ -376,12 +454,38 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ CorePackerVersionString: lockedVersion, Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, Basedir: filepath.Join("testdata", "variables"), }, true, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, }, @@ -481,7 +585,25 @@ func TestParse_variables(t *testing.T) { CorePackerVersionString: lockedVersion, Basedir: filepath.Join("testdata", "variables", "validation"), Builds: Builds{ - &BuildBlock{}, + &BuildBlock{ + Sources: []SourceUseBlock{ + { + SourceRef: SourceRef{ + Type: "null", + Name: "test", + }, + }, + }, + }, + }, + Sources: map[SourceRef]SourceBlock{ + { + Type: "null", + Name: "test", + }: { + Type: "null", + Name: "test", + }, }, InputVariables: Variables{ "image_id": &Variable{ @@ -499,7 +621,15 @@ func TestParse_variables(t *testing.T) { }, }, false, false, - []packersdk.Build{}, + []packersdk.Build{ + &packer.CoreBuild{ + Type: "null.test", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, + }, + }, false, },