From 5ef1893b0067fc86ba091046d897c3d28411fe60 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 14:29:43 +0100 Subject: [PATCH 1/6] fix crash when build.sources is set to an invalid name the `build` body doesn't have any labels and we were trying to display those. I also added a test. --- .../testdata/build/invalid_source_reference.pkr.hcl | 4 ++++ hcl2template/types.build.go | 9 +++++---- hcl2template/types.build_test.go | 10 ++++++++++ 3 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 hcl2template/testdata/build/invalid_source_reference.pkr.hcl diff --git a/hcl2template/testdata/build/invalid_source_reference.pkr.hcl b/hcl2template/testdata/build/invalid_source_reference.pkr.hcl new file mode 100644 index 000000000..a1c28dae9 --- /dev/null +++ b/hcl2template/testdata/build/invalid_source_reference.pkr.hcl @@ -0,0 +1,4 @@ + +build { + sources = ["ami"] +} diff --git a/hcl2template/types.build.go b/hcl2template/types.build.go index c6a2f68e8..2c7170918 100644 --- a/hcl2template/types.build.go +++ b/hcl2template/types.build.go @@ -51,11 +51,11 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti if ref == NoSource { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Invalid " + sourceLabel + " reference", + Summary: "Invalid " + sourceLabel + " reference: " + buildFrom, Detail: "A " + sourceLabel + " type must start with a letter and " + "may contain only letters, digits, underscores, and dashes." + "A valid source reference looks like: `source.type.name`", - Subject: &block.LabelRanges[0], + Subject: block.DefRange.Ptr(), }) continue } @@ -67,7 +67,7 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti Detail: "A " + sourceLabel + " type must start with a letter and " + "may contain only letters, digits, underscores, and dashes." + "A valid source reference looks like: `source.type.name`", - Subject: &block.LabelRanges[0], + Subject: block.DefRange.Ptr(), }) continue } @@ -75,7 +75,8 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti build.Froms = append(build.Froms, ref) } - content, diags := b.Config.Content(buildSchema) + content, moreDiags := b.Config.Content(buildSchema) + diags = append(diags, moreDiags...) for _, block := range content.Blocks { switch block.Type { case buildProvisionerLabel: diff --git a/hcl2template/types.build_test.go b/hcl2template/types.build_test.go index f5e1cea69..a852432ea 100644 --- a/hcl2template/types.build_test.go +++ b/hcl2template/types.build_test.go @@ -83,6 +83,16 @@ func TestParse_build(t *testing.T) { []packer.Build{}, false, }, + {"invalid source", + defaultParser, + parseTestArgs{"testdata/build/invalid_source_reference.pkr.hcl"}, + &PackerConfig{ + Builds: nil, + }, + true, true, + []packer.Build{}, + false, + }, } testParse(t, tests) } From d8c33bc59ef4dfc369d4ce36aa423617ee894700 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 14:31:15 +0100 Subject: [PATCH 2/6] HCL: deshadown warnings from Parser.Parse the func would return when `diags.HasErrors()` but this does not mean that diags is empty. --- hcl2template/types.packer_config.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index dc8f29536..f3f061aa2 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -103,5 +103,6 @@ func (p *Parser) Parse(path string) ([]packer.Build, hcl.Diagnostics) { return nil, diags } - return p.getBuilds(cfg) + builds, moreDiags := p.getBuilds(cfg) + return builds, append(diags, moreDiags...) } From aad90842b4b48e2c0e1f20e8e21f54729f0d4b64 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 16:05:30 +0100 Subject: [PATCH 3/6] add test case and fix the breaking ones --- hcl2template/parser.go | 38 +----- .../testdata/build.pkr.hcl/basic.pkr.hcl | 125 ++++++++++++++++++ hcl2template/types.build_test.go | 2 +- hcl2template/types.packer_config_test.go | 46 ++++++- hcl2template/utils.go | 69 +++++++++- 5 files changed, 242 insertions(+), 38 deletions(-) create mode 100644 hcl2template/testdata/build.pkr.hcl/basic.pkr.hcl diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 770c7209a..0bf4ce261 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -2,9 +2,6 @@ package hcl2template import ( "fmt" - "io/ioutil" - "path/filepath" - "strings" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclparse" @@ -37,39 +34,14 @@ type Parser struct { PostProcessorsSchemas packer.PostProcessorStore } -const hcl2FileExt = ".pkr.hcl" +const ( + hcl2FileExt = ".pkr.hcl" + hcl2JsonFileExt = ".pkr.json" +) func (p *Parser) parse(filename string) (*PackerConfig, hcl.Diagnostics) { - var diags hcl.Diagnostics - hclFiles := []string{} - jsonFiles := []string{} - if strings.HasSuffix(filename, hcl2FileExt) { - hclFiles = append(hclFiles, filename) - } else if strings.HasSuffix(filename, ".json") { - jsonFiles = append(jsonFiles, filename) - } else { - fileInfos, err := ioutil.ReadDir(filename) - if err != nil { - diag := &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Cannot read hcl directory", - Detail: err.Error(), - } - diags = append(diags, diag) - } - for _, fileInfo := range fileInfos { - if fileInfo.IsDir() { - continue - } - filename := filepath.Join(filename, fileInfo.Name()) - if strings.HasSuffix(filename, hcl2FileExt) { - hclFiles = append(hclFiles, filename) - } else if strings.HasSuffix(filename, ".json") { - jsonFiles = append(jsonFiles, filename) - } - } - } + hclFiles, jsonFiles, diags := GetHCL2Files(filename) var files []*hcl.File for _, filename := range hclFiles { diff --git a/hcl2template/testdata/build.pkr.hcl/basic.pkr.hcl b/hcl2template/testdata/build.pkr.hcl/basic.pkr.hcl new file mode 100644 index 000000000..34d84edb9 --- /dev/null +++ b/hcl2template/testdata/build.pkr.hcl/basic.pkr.hcl @@ -0,0 +1,125 @@ + +// starts resources to provision them. +build { + sources = [ + "source.amazon-ebs.ubuntu-1604", + "source.virtualbox-iso.ubuntu-1204", + ] + + provisioner "shell" { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + + nested { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + } + + nested_slice { + } + } + + provisioner "file" { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + + nested { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + } + + nested_slice { + } + } + + post-processor "amazon-import" { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + + nested { + string = "string" + int = 42 + int64 = 43 + bool = true + trilean = true + duration = "10s" + map_string_string { + a = "b" + c = "d" + } + slice_string = [ + "a", + "b", + "c", + ] + } + + nested_slice { + } + } +} diff --git a/hcl2template/types.build_test.go b/hcl2template/types.build_test.go index a852432ea..2a346c84c 100644 --- a/hcl2template/types.build_test.go +++ b/hcl2template/types.build_test.go @@ -21,7 +21,7 @@ func TestParse_build(t *testing.T) { Type: "amazon-ebs", Name: "ubuntu-1604", }, - ref, + refVBIsoUbuntu1204, }, ProvisionerBlocks: []*ProvisionerBlock{ { diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index b47ffba4a..0a04a49fb 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -6,7 +6,10 @@ import ( "github.com/hashicorp/packer/packer" ) -var ref = SourceRef{Type: "virtualbox-iso", Name: "ubuntu-1204"} +var ( + refVBIsoUbuntu1204 = SourceRef{Type: "virtualbox-iso", Name: "ubuntu-1204"} + refAWSEBSUbuntu1204 = SourceRef{Type: "amazon-ebs", Name: "ubuntu-1604"} +) func TestParser_complete(t *testing.T) { defaultParser := getBasicParser() @@ -17,11 +20,11 @@ func TestParser_complete(t *testing.T) { parseTestArgs{"testdata/complete"}, &PackerConfig{ Sources: map[SourceRef]*Source{ - ref: &Source{Type: "virtualbox-iso", Name: "ubuntu-1204"}, + refVBIsoUbuntu1204: &Source{Type: "virtualbox-iso", Name: "ubuntu-1204"}, }, Builds: Builds{ &BuildBlock{ - Froms: []SourceRef{ref}, + Froms: []SourceRef{refVBIsoUbuntu1204}, ProvisionerBlocks: []*ProvisionerBlock{ {PType: "shell"}, {PType: "file"}, @@ -50,6 +53,43 @@ func TestParser_complete(t *testing.T) { }, false, }, + {"dir with no config files", + defaultParser, + parseTestArgs{"testdata/"}, + nil, + true, true, + nil, + false, + }, + {name: "inexistent dir", + parser: defaultParser, + args: parseTestArgs{"testdata/inexistent"}, + parseWantCfg: nil, + parseWantDiags: true, + parseWantDiagHasErrors: true, + }, + {name: "folder named build.pkr.hcl with an unknown src", + parser: defaultParser, + args: parseTestArgs{"testdata/build.pkr.hcl"}, + parseWantCfg: &PackerConfig{ + Builds: Builds{ + &BuildBlock{ + Froms: []SourceRef{refAWSEBSUbuntu1204, refVBIsoUbuntu1204}, + ProvisionerBlocks: []*ProvisionerBlock{ + {PType: "shell"}, + {PType: "file"}, + }, + PostProcessors: []*PostProcessorBlock{ + {PType: "amazon-import"}, + }, + }, + }, + }, + parseWantDiags: false, + parseWantDiagHasErrors: false, + getBuildsWantBuilds: []packer.Build{}, + getBuildsWantDiags: true, + }, } testParse(t, tests) } diff --git a/hcl2template/utils.go b/hcl2template/utils.go index 6edb52447..52429321b 100644 --- a/hcl2template/utils.go +++ b/hcl2template/utils.go @@ -1,6 +1,13 @@ package hcl2template -import "github.com/hashicorp/hcl/v2" +import ( + "io/ioutil" + "os" + "path/filepath" + "strings" + + "github.com/hashicorp/hcl/v2" +) func warningErrorsToDiags(block *hcl.Block, warnings []string, err error) hcl.Diagnostics { var diags hcl.Diagnostics @@ -21,3 +28,63 @@ func warningErrorsToDiags(block *hcl.Block, warnings []string, err error) hcl.Di } return diags } + +func isDir(name string) (bool, error) { + s, err := os.Stat(name) + if err != nil { + return false, err + } + return s.IsDir(), nil +} + +func GetHCL2Files(filename string) (hclFiles, jsonFiles []string, diags hcl.Diagnostics) { + isDir, err := isDir(filename) + if err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot tell wether " + filename + " is a directory", + Detail: err.Error(), + }) + return nil, nil, diags + } + if !isDir { + if strings.HasSuffix(filename, hcl2JsonFileExt) { + return nil, []string{filename}, diags + } + if strings.HasSuffix(filename, hcl2FileExt) { + return []string{filename}, nil, diags + } + } + + fileInfos, err := ioutil.ReadDir(filename) + if err != nil { + diag := &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Cannot read hcl directory", + Detail: err.Error(), + } + diags = append(diags, diag) + return nil, nil, diags + } + for _, fileInfo := range fileInfos { + if fileInfo.IsDir() { + continue + } + filename := filepath.Join(filename, fileInfo.Name()) + if strings.HasSuffix(filename, hcl2FileExt) { + hclFiles = append(hclFiles, filename) + } else if strings.HasSuffix(filename, hcl2JsonFileExt) { + jsonFiles = append(jsonFiles, filename) + } + } + if len(hclFiles)+len(jsonFiles) == 0 { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Could not find any config file in " + filename, + Detail: "A config file must be suffixed with `.pkr.hcl` or " + + "`.pkr.json`. A folder can be referenced.", + }) + } + + return hclFiles, jsonFiles, diags +} From 219c80b1e6bcc3e206af05dbbc4b15e5db44cd64 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 16:09:44 +0100 Subject: [PATCH 4/6] better empty folder test --- hcl2template/testdata/empty/.gitkeep | 0 hcl2template/types.packer_config_test.go | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 hcl2template/testdata/empty/.gitkeep diff --git a/hcl2template/testdata/empty/.gitkeep b/hcl2template/testdata/empty/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 0a04a49fb..38afe10da 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -55,7 +55,7 @@ func TestParser_complete(t *testing.T) { }, {"dir with no config files", defaultParser, - parseTestArgs{"testdata/"}, + parseTestArgs{"testdata/empty"}, nil, true, true, nil, From 0102f5b6fad75c0185b1d287bd676e26111a6966 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 16:56:42 +0100 Subject: [PATCH 5/6] remove unecessary inexistant block panic + add test the hcl library will error for us --- hcl2template/parser.go | 2 -- hcl2template/testdata/unknown/block_type.pkr.hcl | 3 +++ hcl2template/types.packer_config_test.go | 7 +++++++ 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 hcl2template/testdata/unknown/block_type.pkr.hcl diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 0bf4ce261..b79c8e3bb 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -127,8 +127,6 @@ func (p *Parser) parseFile(f *hcl.File, cfg *PackerConfig) hcl.Diagnostics { } cfg.Builds = append(cfg.Builds, build) - default: - panic(fmt.Sprintf("unexpected block type %q", block.Type)) // TODO(azr): err } } diff --git a/hcl2template/testdata/unknown/block_type.pkr.hcl b/hcl2template/testdata/unknown/block_type.pkr.hcl new file mode 100644 index 000000000..8ae2c40a3 --- /dev/null +++ b/hcl2template/testdata/unknown/block_type.pkr.hcl @@ -0,0 +1,3 @@ + +potato { +} diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 38afe10da..0e410a850 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -90,6 +90,13 @@ func TestParser_complete(t *testing.T) { getBuildsWantBuilds: []packer.Build{}, getBuildsWantDiags: true, }, + {name: "unknown block type", + parser: defaultParser, + args: parseTestArgs{"testdata/unknown"}, + parseWantCfg: &PackerConfig{}, + parseWantDiags: true, + parseWantDiagHasErrors: true, + }, } testParse(t, tests) } From 79d7b3e636be2efb531518dccbc330398ccad7d8 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 6 Jan 2020 17:10:12 +0100 Subject: [PATCH 6/6] regroup duplicate error message --- hcl2template/types.build.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/hcl2template/types.build.go b/hcl2template/types.build.go index 2c7170918..6f3485b95 100644 --- a/hcl2template/types.build.go +++ b/hcl2template/types.build.go @@ -48,23 +48,14 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti for _, buildFrom := range b.FromSources { ref := sourceRefFromString(buildFrom) - if ref == NoSource { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid " + sourceLabel + " reference: " + buildFrom, - Detail: "A " + sourceLabel + " type must start with a letter and " + - "may contain only letters, digits, underscores, and dashes." + - "A valid source reference looks like: `source.type.name`", - Subject: block.DefRange.Ptr(), - }) - continue - } - if !hclsyntax.ValidIdentifier(ref.Type) || + if ref == NoSource || + !hclsyntax.ValidIdentifier(ref.Type) || !hclsyntax.ValidIdentifier(ref.Name) { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid " + sourceLabel + " reference", - Detail: "A " + sourceLabel + " type must start with a letter and " + + Detail: "A " + sourceLabel + " type is made of three parts that are" + + "split by a dot `.`; each part must start with a letter and " + "may contain only letters, digits, underscores, and dashes." + "A valid source reference looks like: `source.type.name`", Subject: block.DefRange.Ptr(),