From 4b10c5a87c28b64f15ae8efb869b977e25e3d35e Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 14 Feb 2020 11:54:10 +0100 Subject: [PATCH 01/21] try to reproduce #8730 in tests --- hcl2template/testdata/complete/build.pkr.hcl | 2 +- hcl2template/testdata/complete/variables.pkr.hcl | 4 ++++ hcl2template/types.packer_config_test.go | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/hcl2template/testdata/complete/build.pkr.hcl b/hcl2template/testdata/complete/build.pkr.hcl index 64cf16c2e..6a01c01bd 100644 --- a/hcl2template/testdata/complete/build.pkr.hcl +++ b/hcl2template/testdata/complete/build.pkr.hcl @@ -8,7 +8,7 @@ build { provisioner "shell" { name = "provisioner that does something" not_squashed = var.foo - string = "string" + string = "string${var.proxmox_username}" int = "${41 + 1}" int64 = "${42 + 1}" bool = "true" diff --git a/hcl2template/testdata/complete/variables.pkr.hcl b/hcl2template/testdata/complete/variables.pkr.hcl index d144cc1cf..db0b68df5 100644 --- a/hcl2template/testdata/complete/variables.pkr.hcl +++ b/hcl2template/testdata/complete/variables.pkr.hcl @@ -10,6 +10,10 @@ variable "image_id" { default = "image-id-default" } +variable "proxmox_username" { + type = string +} + variable "port" { type = number default = 42 diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 38fed400a..f5019258f 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -25,6 +25,7 @@ func TestParser_complete(t *testing.T) { "image_id": &Variable{}, "port": &Variable{}, "availability_zone_names": &Variable{}, + "proxmox_username": &Variable{}, }, LocalVariables: Variables{ "feefoo": &Variable{}, From 1ec68cac23be19ecb8aff4e84dfbef00117bcd17 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 17 Feb 2020 15:47:31 +0100 Subject: [PATCH 02/21] refactor prepare into MockConfig --- hcl2template/internal/mock.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/hcl2template/internal/mock.go b/hcl2template/internal/mock.go index 632465ae0..84d00d98a 100644 --- a/hcl2template/internal/mock.go +++ b/hcl2template/internal/mock.go @@ -32,6 +32,12 @@ type MockConfig struct { NestedSlice []NestedMockConfig `mapstructure:"nested_slice"` } +func (b *MockConfig) Prepare(raws ...interface{}) error { + return config.Decode(b, &config.DecodeOpts{ + Interpolate: true, + }, raws...) +} + ////// // MockBuilder ////// @@ -45,9 +51,7 @@ var _ packer.Builder = new(MockBuilder) func (b *MockBuilder) ConfigSpec() hcldec.ObjectSpec { return b.Config.FlatMapstructure().HCL2Spec() } func (b *MockBuilder) Prepare(raws ...interface{}) ([]string, []string, error) { - return nil, nil, config.Decode(&b.Config, &config.DecodeOpts{ - Interpolate: true, - }, raws...) + return nil, nil, b.Config.Prepare(raws...) } func (b *MockBuilder) Run(ctx context.Context, ui packer.Ui, hook packer.Hook) (packer.Artifact, error) { @@ -69,9 +73,7 @@ func (b *MockProvisioner) ConfigSpec() hcldec.ObjectSpec { } func (b *MockProvisioner) Prepare(raws ...interface{}) error { - return config.Decode(&b.Config, &config.DecodeOpts{ - Interpolate: true, - }, raws...) + return b.Config.Prepare(raws...) } func (b *MockProvisioner) Provision(ctx context.Context, ui packer.Ui, comm packer.Communicator, _ map[string]interface{}) error { @@ -93,9 +95,7 @@ func (b *MockPostProcessor) ConfigSpec() hcldec.ObjectSpec { } func (b *MockPostProcessor) Configure(raws ...interface{}) error { - return config.Decode(&b.Config, &config.DecodeOpts{ - Interpolate: true, - }, raws...) + return b.Config.Prepare(raws...) } func (b *MockPostProcessor) PostProcess(ctx context.Context, ui packer.Ui, a packer.Artifact) (packer.Artifact, bool, bool, error) { @@ -118,9 +118,7 @@ func (b *MockCommunicator) ConfigSpec() hcldec.ObjectSpec { } func (b *MockCommunicator) Configure(raws ...interface{}) ([]string, error) { - return nil, config.Decode(&b.Config, &config.DecodeOpts{ - Interpolate: true, - }, raws...) + return nil, b.Config.Prepare(raws...) } ////// From 00c812cfe8de18074784ea32ec8811accbcbc345 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 17 Feb 2020 15:53:05 +0100 Subject: [PATCH 03/21] insert "github.com/zclconf/go-cty/cty/json" encoding beforce hcl decoding things to make sure tests are working similarly as real life version --- hcl2template/internal/mock.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/hcl2template/internal/mock.go b/hcl2template/internal/mock.go index 84d00d98a..0900c679a 100644 --- a/hcl2template/internal/mock.go +++ b/hcl2template/internal/mock.go @@ -6,9 +6,12 @@ import ( "context" "time" + "github.com/zclconf/go-cty/cty/json" + "github.com/hashicorp/hcl/v2/hcldec" "github.com/hashicorp/packer/helper/config" "github.com/hashicorp/packer/packer" + "github.com/zclconf/go-cty/cty" ) type NestedMockConfig struct { @@ -33,6 +36,21 @@ type MockConfig struct { } func (b *MockConfig) Prepare(raws ...interface{}) error { + for i, raw := range raws { + cval, ok := raw.(cty.Value) + if !ok { + continue + } + b, err := json.Marshal(cval, cty.DynamicPseudoType) + if err != nil { + return err + } + ccval, err := json.Unmarshal(b, cty.DynamicPseudoType) + if err != nil { + return err + } + raws[i] = ccval + } return config.Decode(b, &config.DecodeOpts{ Interpolate: true, }, raws...) From 79867ca26ebfa77323487955fa1b777ecffd7b81 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 17 Feb 2020 16:36:19 +0100 Subject: [PATCH 04/21] add test for unset variable --- hcl2template/common_test.go | 2 ++ .../variables/unset_string_variable.pkr.hcl | 14 +++++++++ hcl2template/types.variables_test.go | 29 +++++++++++++++++++ 3 files changed, 45 insertions(+) create mode 100644 hcl2template/testdata/variables/unset_string_variable.pkr.hcl diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index e930c3b74..c3b17b817 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclparse" + "github.com/hashicorp/packer/builder/null" . "github.com/hashicorp/packer/hcl2template/internal" "github.com/hashicorp/packer/helper/config" "github.com/hashicorp/packer/packer" @@ -21,6 +22,7 @@ func getBasicParser() *Parser { BuilderSchemas: packer.MapOfBuilder{ "amazon-ebs": func() (packer.Builder, error) { return &MockBuilder{}, nil }, "virtualbox-iso": func() (packer.Builder, error) { return &MockBuilder{}, nil }, + "null": func() (packer.Builder, error) { return &null.Builder{}, nil }, }, ProvisionersSchemas: packer.MapOfProvisioner{ "shell": func() (packer.Provisioner, error) { return &MockProvisioner{}, nil }, diff --git a/hcl2template/testdata/variables/unset_string_variable.pkr.hcl b/hcl2template/testdata/variables/unset_string_variable.pkr.hcl new file mode 100644 index 000000000..3f4a9c7cb --- /dev/null +++ b/hcl2template/testdata/variables/unset_string_variable.pkr.hcl @@ -0,0 +1,14 @@ + +variable "foo" { + type = string +} + +build { + sources = [ + "source.null.null-builder", + ] +} + +source "null" "null-builder" { + communicator = var.foo +} diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index bafd371f2..fc6e6706d 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -10,6 +10,7 @@ import ( "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/packer/builder/null" "github.com/hashicorp/packer/packer" ) @@ -97,6 +98,34 @@ func TestParse_variables(t *testing.T) { []packer.Build{}, false, }, + {"unset variable", + defaultParser, + parseTestArgs{"testdata/variables/unset_string_variable.pkr.hcl", nil}, + &PackerConfig{ + Basedir: filepath.Join("testdata", "variables"), + InputVariables: Variables{ + "foo": &Variable{}, + }, + Sources: map[SourceRef]*SourceBlock{ + SourceRef{"null", "null-builder"}: &SourceBlock{ + Name: "null-builder", + Type: "null", + }, + }, + Builds: Builds{ + &BuildBlock{ + Sources: []SourceRef{SourceRef{"null", "null-builder"}}, + }, + }, + }, + false, false, + []packer.Build{ + &packer.CoreBuild{ + Builder: &null.Builder{}, + }, + }, + false, + }, } testParse(t, tests) } From cebfb1c735dc0fd91e3e1d2cf534c4b66deec8ff Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 17 Feb 2020 17:15:52 +0100 Subject: [PATCH 05/21] give correct error when variable is unset --- hcl2template/parser.go | 5 +++++ hcl2template/types.build.go | 6 ++++++ hcl2template/types.packer_config.go | 6 ++++-- hcl2template/types.variables.go | 14 +++++++++----- hcl2template/types.variables_test.go | 9 ++------- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 6f7467da5..cf7af9fd4 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -101,6 +101,11 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, } } + _, moreDiags := cfg.InputVariables.Values() + diags = append(diags, moreDiags...) + _, moreDiags = cfg.LocalVariables.Values() + diags = append(diags, moreDiags...) + // parse var files { hclVarFiles, jsonVarFiles, moreDiags := GetHCL2Files(filename, hcl2VarFileExt, hcl2VarJsonFileExt) diff --git a/hcl2template/types.build.go b/hcl2template/types.build.go index 47f833ddf..09b663525 100644 --- a/hcl2template/types.build.go +++ b/hcl2template/types.build.go @@ -60,6 +60,9 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti Config hcl.Body `hcl:",remain"` } diags := gohcl.DecodeBody(block.Body, nil, &b) + if diags.HasErrors() { + return nil, diags + } for _, buildFrom := range b.FromSources { ref := sourceRefFromString(buildFrom) @@ -84,6 +87,9 @@ func (p *Parser) decodeBuildConfig(block *hcl.Block) (*BuildBlock, hcl.Diagnosti content, moreDiags := b.Config.Content(buildSchema) diags = append(diags, moreDiags...) + if diags.HasErrors() { + return nil, diags + } for _, block := range content.Blocks { switch block.Type { case buildProvisionerLabel: diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 6e003d50a..e68405643 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -30,11 +30,13 @@ type PackerConfig struct { // decoder in order to tell what is the actual value of a var or a local and // the list of defined functions. func (cfg *PackerConfig) EvalContext() *hcl.EvalContext { + v, _ := cfg.InputVariables.Values() + l, _ := cfg.LocalVariables.Values() ectx := &hcl.EvalContext{ Functions: Functions(cfg.Basedir), Variables: map[string]cty.Value{ - "var": cty.ObjectVal(cfg.InputVariables.Values()), - "local": cty.ObjectVal(cfg.LocalVariables.Values()), + "var": cty.ObjectVal(v), + "local": cty.ObjectVal(l), }, } return ectx diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index d05d3a841..cb74ff602 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -56,10 +56,11 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { return value, nil } } - return cty.NilVal, &hcl.Diagnostic{ + + return cty.UnknownVal(v.Type), &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Unset variable", - Detail: "A used variable must be set; see " + + Detail: "A used variable must be set or have a default value; see " + "https://packer.io/docs/configuration/from-1.5/syntax.html for details.", Context: v.block.DefRange.Ptr(), } @@ -67,12 +68,15 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { type Variables map[string]*Variable -func (variables Variables) Values() map[string]cty.Value { +func (variables Variables) Values() (map[string]cty.Value, hcl.Diagnostics) { res := map[string]cty.Value{} + var diags hcl.Diagnostics for k, v := range variables { - res[k], _ = v.Value() + var diag *hcl.Diagnostic + res[k], diag = v.Value() + diags = append(diags, diag) } - return res + return res, diags } // decodeConfig decodes a "variables" section the way packer 1 used to diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index fc6e6706d..c54c48978 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -10,7 +10,6 @@ import ( "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/hcl/v2" - "github.com/hashicorp/packer/builder/null" "github.com/hashicorp/packer/packer" ) @@ -119,12 +118,8 @@ func TestParse_variables(t *testing.T) { }, }, false, false, - []packer.Build{ - &packer.CoreBuild{ - Builder: &null.Builder{}, - }, - }, - false, + []packer.Build{}, + true, }, } testParse(t, tests) From 3c213e6eaf0537993bf63fb4b3b24aac44c8a663 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 17 Feb 2020 18:05:15 +0100 Subject: [PATCH 06/21] continue and realise and unused undefaulted value triggers an error --- hcl2template/testdata/complete/build.pkr.hcl | 2 +- .../testdata/complete/variables.pkr.hcl | 4 -- hcl2template/types.packer_config_test.go | 21 +++++++--- hcl2template/types.variables.go | 15 ++++++-- hcl2template/types.variables_test.go | 38 ++++++++++++------- 5 files changed, 53 insertions(+), 27 deletions(-) diff --git a/hcl2template/testdata/complete/build.pkr.hcl b/hcl2template/testdata/complete/build.pkr.hcl index 6a01c01bd..64cf16c2e 100644 --- a/hcl2template/testdata/complete/build.pkr.hcl +++ b/hcl2template/testdata/complete/build.pkr.hcl @@ -8,7 +8,7 @@ build { provisioner "shell" { name = "provisioner that does something" not_squashed = var.foo - string = "string${var.proxmox_username}" + string = "string" int = "${41 + 1}" int64 = "${42 + 1}" bool = "true" diff --git a/hcl2template/testdata/complete/variables.pkr.hcl b/hcl2template/testdata/complete/variables.pkr.hcl index db0b68df5..d144cc1cf 100644 --- a/hcl2template/testdata/complete/variables.pkr.hcl +++ b/hcl2template/testdata/complete/variables.pkr.hcl @@ -10,10 +10,6 @@ variable "image_id" { default = "image-id-default" } -variable "proxmox_username" { - type = string -} - variable "port" { type = number default = 42 diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index f5019258f..76156ffa6 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -21,14 +21,23 @@ func TestParser_complete(t *testing.T) { &PackerConfig{ Basedir: "testdata/complete", InputVariables: Variables{ - "foo": &Variable{}, - "image_id": &Variable{}, - "port": &Variable{}, - "availability_zone_names": &Variable{}, - "proxmox_username": &Variable{}, + "foo": &Variable{ + Name: "foo", + }, + "image_id": &Variable{ + Name: "image_id", + }, + "port": &Variable{ + Name: "port", + }, + "availability_zone_names": &Variable{ + Name: "availability_zone_names", + }, }, LocalVariables: Variables{ - "feefoo": &Variable{}, + "feefoo": &Variable{ + Name: "feefoo", + }, }, Sources: map[SourceRef]*SourceBlock{ refVBIsoUbuntu1204: {Type: "virtualbox-iso", Name: "ubuntu-1204"}, diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index cb74ff602..29112cda7 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -31,6 +31,8 @@ type Variable struct { // declaration, the type of the default variable will be used. This will // allow to ensure that users set this variable correctly. Type cty.Type + // Common name of the variable + Name string // Description of the variable Description string // When Sensitive is set to true Packer will try it best to hide/obfuscate @@ -59,7 +61,7 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { return cty.UnknownVal(v.Type), &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Unset variable", + Summary: fmt.Sprintf("Unset variable %q", v.Name), Detail: "A used variable must be set or have a default value; see " + "https://packer.io/docs/configuration/from-1.5/syntax.html for details.", Context: v.block.DefRange.Ptr(), @@ -74,7 +76,9 @@ func (variables Variables) Values() (map[string]cty.Value, hcl.Diagnostics) { for k, v := range variables { var diag *hcl.Diagnostic res[k], diag = v.Value() - diags = append(diags, diag) + if diag != nil { + diags = append(diags, diag) + } } return res, diags } @@ -107,8 +111,10 @@ func (variables *Variables) decodeConfigMap(block *hcl.Block, ectx *hcl.EvalCont continue } (*variables)[key] = &Variable{ + Name: key, DefaultValue: value, Type: value.Type(), + block: block, } } @@ -142,7 +148,10 @@ func (variables *Variables) decodeConfig(block *hcl.Block, ectx *hcl.EvalContext return diags } + name := block.Labels[0] + res := &Variable{ + Name: name, Description: b.Description, Sensitive: b.Sensitive, block: block, @@ -200,7 +209,7 @@ func (variables *Variables) decodeConfig(block *hcl.Block, ectx *hcl.EvalContext }) } - (*variables)[block.Labels[0]] = res + (*variables)[name] = res return diags } diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index c54c48978..687f8f5a4 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -23,15 +23,17 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "image_name": &Variable{}, - "key": &Variable{}, - "my_secret": &Variable{}, - "image_id": &Variable{}, - "port": &Variable{}, + "image_name": &Variable{Name: "image_name"}, + "key": &Variable{Name: "key"}, + "my_secret": &Variable{Name: "my_secret"}, + "image_id": &Variable{Name: "image_id"}, + "port": &Variable{Name: "port"}, "availability_zone_names": &Variable{ + Name: "availability_zone_names", Description: fmt.Sprintln("Describing is awesome ;D"), }, "super_secret_password": &Variable{ + Name: "super_secret_password", Sensitive: true, Description: fmt.Sprintln("Handle with care plz"), }, @@ -51,7 +53,9 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "boolean_value": &Variable{}, + "boolean_value": &Variable{ + Name: "boolean_value", + }, }, }, true, true, @@ -64,7 +68,9 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "boolean_value": &Variable{}, + "boolean_value": &Variable{ + Name: "boolean_value", + }, }, }, true, true, @@ -77,23 +83,27 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "broken_type": &Variable{}, + "broken_type": &Variable{ + Name: "broken_type", + }, }, }, true, true, []packer.Build{}, false, }, - {"invalid default type", + {"unknown key", defaultParser, parseTestArgs{"testdata/variables/unknown_key.pkr.hcl", nil}, &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "broken_type": &Variable{}, + "broken_type": &Variable{ + Name: "broken_type", + }, }, }, - true, false, + true, true, []packer.Build{}, false, }, @@ -103,7 +113,9 @@ func TestParse_variables(t *testing.T) { &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ - "foo": &Variable{}, + "foo": &Variable{ + Name: "foo", + }, }, Sources: map[SourceRef]*SourceBlock{ SourceRef{"null", "null-builder"}: &SourceBlock{ @@ -117,7 +129,7 @@ func TestParse_variables(t *testing.T) { }, }, }, - false, false, + true, true, []packer.Build{}, true, }, From 52e5b7051e0381aaa6840805394081edaa465478 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 18 Feb 2020 11:31:52 +0100 Subject: [PATCH 07/21] Update mock.go --- hcl2template/internal/mock.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hcl2template/internal/mock.go b/hcl2template/internal/mock.go index 0900c679a..bc82bb067 100644 --- a/hcl2template/internal/mock.go +++ b/hcl2template/internal/mock.go @@ -6,12 +6,11 @@ import ( "context" "time" - "github.com/zclconf/go-cty/cty/json" - "github.com/hashicorp/hcl/v2/hcldec" "github.com/hashicorp/packer/helper/config" "github.com/hashicorp/packer/packer" "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/json" ) type NestedMockConfig struct { From 50896d4ddf1521a8bf3ed683eb5e57e8d0b4ebe0 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 3 Mar 2020 11:15:56 +0100 Subject: [PATCH 08/21] WIP --- hcl2template/common_test.go | 3 +- hcl2template/parser.go | 8 ++-- .../unset_unused_string_variable.pkr.hcl | 15 ++++++ ...hcl => unset_used_string_variable.pkr.hcl} | 0 hcl2template/types.variables.go | 8 +++- hcl2template/types.variables_test.go | 46 ++++++++++++++++--- 6 files changed, 68 insertions(+), 12 deletions(-) create mode 100644 hcl2template/testdata/variables/unset_unused_string_variable.pkr.hcl rename hcl2template/testdata/variables/{unset_string_variable.pkr.hcl => unset_used_string_variable.pkr.hcl} (100%) diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index c3b17b817..fe2a8afc7 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -60,7 +60,7 @@ func testParse(t *testing.T, tests []parseTest) { t.Run(tt.name, func(t *testing.T) { gotCfg, gotDiags := tt.parser.parse(tt.args.filename, tt.args.vars) if tt.parseWantDiags == (gotDiags == nil) { - t.Fatalf("Parser.parse() unexpected diagnostics. %s", gotDiags) + t.Fatalf("Parser.parse() unexpected %q diagnostics.", gotDiags) } if tt.parseWantDiagHasErrors != gotDiags.HasErrors() { t.Fatalf("Parser.parse() unexpected diagnostics HasErrors. %s", gotDiags) @@ -97,6 +97,7 @@ func testParse(t *testing.T, tests []parseTest) { packer.CoreBuild{}, packer.CoreBuildProvisioner{}, packer.CoreBuildPostProcessor{}, + null.Builder{}, ), ); diff != "" { t.Fatalf("Parser.getBuilds() wrong packer builds. %s", diff) diff --git a/hcl2template/parser.go b/hcl2template/parser.go index cf7af9fd4..071bf6206 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -101,10 +101,10 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, } } - _, moreDiags := cfg.InputVariables.Values() - diags = append(diags, moreDiags...) - _, moreDiags = cfg.LocalVariables.Values() - diags = append(diags, moreDiags...) + // _, moreDiags := cfg.InputVariables.Values() + // diags = append(diags, moreDiags...) + // _, moreDiags = cfg.LocalVariables.Values() + // diags = append(diags, moreDiags...) // parse var files { diff --git a/hcl2template/testdata/variables/unset_unused_string_variable.pkr.hcl b/hcl2template/testdata/variables/unset_unused_string_variable.pkr.hcl new file mode 100644 index 000000000..1853a09d2 --- /dev/null +++ b/hcl2template/testdata/variables/unset_unused_string_variable.pkr.hcl @@ -0,0 +1,15 @@ + +variable "foo" { + type = string +} + + +build { + sources = [ + "source.null.null-builder", + ] +} + +source "null" "null-builder" { + communicator = "none" +} diff --git a/hcl2template/testdata/variables/unset_string_variable.pkr.hcl b/hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl similarity index 100% rename from hcl2template/testdata/variables/unset_string_variable.pkr.hcl rename to hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 29112cda7..29e4d51d6 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -59,7 +59,12 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { } } - return cty.UnknownVal(v.Type), &hcl.Diagnostic{ + value := cty.NullVal(cty.DynamicPseudoType) + if v.Type != cty.NilType { + cty.NullVal(v.Type) + } + + return value, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: fmt.Sprintf("Unset variable %q", v.Name), Detail: "A used variable must be set or have a default value; see " + @@ -248,6 +253,7 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File if moreDiags.HasErrors() { continue } + val, valDiags := expr.Value(nil) diags = append(diags, valDiags...) diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index 687f8f5a4..8c3894927 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -10,6 +10,7 @@ import ( "github.com/zclconf/go-cty/cty/convert" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/packer/builder/null" "github.com/hashicorp/packer/packer" ) @@ -39,8 +40,8 @@ func TestParse_variables(t *testing.T) { }, }, LocalVariables: Variables{ - "owner": &Variable{}, - "service_name": &Variable{}, + "owner": &Variable{Name: "owner"}, + "service_name": &Variable{Name: "service_name"}, }, }, false, false, @@ -103,13 +104,13 @@ func TestParse_variables(t *testing.T) { }, }, }, - true, true, + true, false, []packer.Build{}, false, }, - {"unset variable", + {"unset used variable", defaultParser, - parseTestArgs{"testdata/variables/unset_string_variable.pkr.hcl", nil}, + parseTestArgs{"testdata/variables/unset_used_string_variable.pkr.hcl", nil}, &PackerConfig{ Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ @@ -131,7 +132,40 @@ func TestParse_variables(t *testing.T) { }, true, true, []packer.Build{}, - true, + false, + }, + {"unset unused variable", + defaultParser, + parseTestArgs{"testdata/variables/unset_unused_string_variable.pkr.hcl", nil}, + &PackerConfig{ + Basedir: filepath.Join("testdata", "variables"), + InputVariables: Variables{ + "foo": &Variable{ + Name: "foo", + }, + }, + Sources: map[SourceRef]*SourceBlock{ + SourceRef{"null", "null-builder"}: &SourceBlock{ + Name: "null-builder", + Type: "null", + }, + }, + Builds: Builds{ + &BuildBlock{ + Sources: []SourceRef{SourceRef{"null", "null-builder"}}, + }, + }, + }, + false, false, + []packer.Build{ + &packer.CoreBuild{ + Type: "null", + Builder: &null.Builder{}, + Provisioners: []packer.CoreBuildProvisioner{}, + PostProcessors: [][]packer.CoreBuildPostProcessor{}, + }, + }, + false, }, } testParse(t, tests) From 382f209b0737c7df64a88c1767135ff70b45f83d Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 12:30:05 +0100 Subject: [PATCH 09/21] Update types.packer_config.go --- hcl2template/types.packer_config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 6694774be..a0897995f 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -2,6 +2,7 @@ package hcl2template import ( "fmt" + "github.com/hashicorp/hcl/v2" "github.com/hashicorp/packer/helper/common" "github.com/hashicorp/packer/packer" From e8510507743c42792575b4c2a073e2112a8075c2 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 12:32:29 +0100 Subject: [PATCH 10/21] don't force to set unused variables too --- hcl2template/parser.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 39b4c76ff..9ecbde9a2 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -107,11 +107,6 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, diags = append(diags, cfg.evaluateLocalVariables(locals)...) } - // _, moreDiags := cfg.InputVariables.Values() - // diags = append(diags, moreDiags...) - // _, moreDiags = cfg.LocalVariables.Values() - // diags = append(diags, moreDiags...) - // parse var files { hclVarFiles, jsonVarFiles, moreDiags := GetHCL2Files(filename, hcl2VarFileExt, hcl2VarJsonFileExt) From 23b940dbd54a346e1ed622f8a56f01ac21f95b8b Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 12:42:56 +0100 Subject: [PATCH 11/21] fix this ! --- hcl2template/types.packer_config.go | 6 ++---- hcl2template/types.variables.go | 18 +++++++----------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index a0897995f..ff889719b 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -33,13 +33,11 @@ type PackerConfig struct { // decoder in order to tell what is the actual value of a var or a local and // the list of defined functions. func (cfg *PackerConfig) EvalContext() *hcl.EvalContext { - v, _ := cfg.InputVariables.Values() - l, _ := cfg.LocalVariables.Values() ectx := &hcl.EvalContext{ Functions: Functions(cfg.Basedir), Variables: map[string]cty.Value{ - "var": cty.ObjectVal(v), - "local": cty.ObjectVal(l), + "var": cty.ObjectVal(cfg.InputVariables.Values()), + "local": cty.ObjectVal(cfg.LocalVariables.Values()), }, } return ectx diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 1b928f07f..b15071e2a 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -68,9 +68,6 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { } value := cty.NullVal(cty.DynamicPseudoType) - if v.Type != cty.NilType { - cty.NullVal(v.Type) - } return value, &hcl.Diagnostic{ Severity: hcl.DiagError, @@ -83,17 +80,16 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { type Variables map[string]*Variable -func (variables Variables) Values() (map[string]cty.Value, hcl.Diagnostics) { +func (variables Variables) Values() map[string]cty.Value { res := map[string]cty.Value{} - var diags hcl.Diagnostics for k, v := range variables { - var diag *hcl.Diagnostic - res[k], diag = v.Value() - if diag != nil { - diags = append(diags, diag) - } + res[k], _ = v.Value() + // here the value might not be used and in that case we don't want to + // force users to set it. So this error is ignored. we still set it as + // it can be a `cty.NullVal(cty.DynamicPseudoType)`, which is the go + // cty value for 'unknown value' and should error when used. } - return res, diags + return res } // decodeVariable decodes a variable key and value into Variables From bd3112fa7c433cf11bf83aa5c248f30933521d4f Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:00:51 +0100 Subject: [PATCH 12/21] parseLocalVariables: simplify loop --- hcl2template/types.packer_config.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index ff889719b..936e1b67f 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -77,20 +77,19 @@ func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*Local, hcl.Diagnosti content, moreDiags := f.Body.Content(configSchema) diags = append(diags, moreDiags...) - var allLocals []*Local + var locals []*Local for _, block := range content.Blocks { switch block.Type { case localsLabel: attrs, moreDiags := block.Body.JustAttributes() diags = append(diags, moreDiags...) - locals := make([]*Local, 0, len(attrs)) for name, attr := range attrs { if _, found := c.LocalVariables[name]; found { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: "Duplicate variable", - Detail: "Duplicate " + name + " variable definition found.", + Summary: "Duplicate value in " + localsLabel, + Detail: "Duplicate " + name + " definition found.", Subject: attr.NameRange.Ptr(), Context: block.DefRange.Ptr(), }) @@ -101,11 +100,10 @@ func (c *PackerConfig) parseLocalVariables(f *hcl.File) ([]*Local, hcl.Diagnosti Expr: attr.Expr, }) } - allLocals = append(allLocals, locals...) } } - return allLocals, diags + return locals, diags } func (c *PackerConfig) evaluateLocalVariables(locals []*Local) hcl.Diagnostics { From 8482c6c2e6489020f134d16085e284cf29f59459 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:01:04 +0100 Subject: [PATCH 13/21] evaluateLocalVariable: also pass the variable name --- hcl2template/types.packer_config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 936e1b67f..5748237b2 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -155,6 +155,7 @@ func (c *PackerConfig) evaluateLocalVariable(local *Local) hcl.Diagnostics { return diags } c.LocalVariables[local.Name] = &Variable{ + Name: local.Name, DefaultValue: value, Type: value.Type(), } From 9b649594c6f6ae7249710fe0aeddce4187a78e13 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:01:18 +0100 Subject: [PATCH 14/21] update tests --- .../unset_used_string_variable.pkr.hcl | 6 +---- hcl2template/types.packer_config_test.go | 5 +++- hcl2template/types.variables_test.go | 23 +++++++++---------- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl b/hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl index 3f4a9c7cb..f8060cccf 100644 --- a/hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl +++ b/hcl2template/testdata/variables/unset_used_string_variable.pkr.hcl @@ -5,10 +5,6 @@ variable "foo" { build { sources = [ - "source.null.null-builder", + "source.null.null-builder${var.foo}", ] } - -source "null" "null-builder" { - communicator = var.foo -} diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 334d9bd8a..d84cef1fc 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -23,7 +23,8 @@ func TestParser_complete(t *testing.T) { Basedir: "testdata/complete", InputVariables: Variables{ "foo": &Variable{ - Name: "foo", + Name: "foo", + DefaultValue: cty.StringVal("value"), }, "image_id": &Variable{ Name: "image_id", @@ -48,12 +49,14 @@ func TestParser_complete(t *testing.T) { DefaultValue: cty.StringVal("value_image-id-default"), }, "standard_tags": &Variable{ + Name: "standard_tags", DefaultValue: cty.ObjectVal(map[string]cty.Value{ "Component": cty.StringVal("user-service"), "Environment": cty.StringVal("production"), }), }, "abc_map": &Variable{ + Name: "abc_map", DefaultValue: cty.TupleVal([]cty.Value{ cty.ObjectVal(map[string]cty.Value{ "id": cty.StringVal("a"), diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index 13ca9298f..b4cd03b78 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -142,22 +142,12 @@ func TestParse_variables(t *testing.T) { Name: "foo", }, }, - Sources: map[SourceRef]*SourceBlock{ - SourceRef{"null", "null-builder"}: &SourceBlock{ - Name: "null-builder", - Type: "null", - }, - }, - Builds: Builds{ - &BuildBlock{ - Sources: []SourceRef{SourceRef{"null", "null-builder"}}, - }, - }, }, true, true, []packer.Build{}, - false, + true, }, + {"unset unused variable", defaultParser, parseTestArgs{"testdata/variables/unset_unused_string_variable.pkr.hcl", nil}, @@ -187,9 +177,12 @@ func TestParse_variables(t *testing.T) { Builder: &null.Builder{}, Provisioners: []packer.CoreBuildProvisioner{}, PostProcessors: [][]packer.CoreBuildPostProcessor{}, + Prepared: true, }, }, + false, }, + {"locals within another locals usage in different files", defaultParser, parseTestArgs{"testdata/variables/complicated", nil}, @@ -197,23 +190,29 @@ func TestParse_variables(t *testing.T) { Basedir: "testdata/variables/complicated", InputVariables: Variables{ "name_prefix": &Variable{ + Name: "name_prefix", DefaultValue: cty.StringVal("foo"), }, }, LocalVariables: Variables{ "name_prefix": &Variable{ + Name: "name_prefix", DefaultValue: cty.StringVal("foo"), }, "foo": &Variable{ + Name: "foo", DefaultValue: cty.StringVal("foo"), }, "bar": &Variable{ + Name: "bar", DefaultValue: cty.StringVal("foo"), }, "for_var": &Variable{ + Name: "for_var", DefaultValue: cty.StringVal("foo"), }, "bar_var": &Variable{ + Name: "bar_var", DefaultValue: cty.TupleVal([]cty.Value{ cty.StringVal("foo"), cty.StringVal("foo"), From e9d3826219bbf4ebd3f4130f9a9742b622b8efd5 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:19:07 +0100 Subject: [PATCH 15/21] variables: use Range instead of Block because a variable can be defined in the Locals block --- hcl2template/types.variables.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index b15071e2a..55158379b 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -47,7 +47,7 @@ type Variable struct { // the variable from the output stream. By replacing the text. Sensitive bool - block *hcl.Block + Range hcl.Range } func (v *Variable) GoString() string { @@ -74,7 +74,7 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { Summary: fmt.Sprintf("Unset variable %q", v.Name), Detail: "A used variable must be set or have a default value; see " + "https://packer.io/docs/configuration/from-1.5/syntax.html for details.", - Context: v.block.DefRange.Ptr(), + Context: v.Range.Ptr(), } } @@ -120,7 +120,7 @@ func (variables *Variables) decodeVariable(key string, attr *hcl.Attribute, ectx Name: key, DefaultValue: value, Type: value.Type(), - // block: attr, + Range: attr.Range, } return diags @@ -159,7 +159,7 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval Name: name, Description: b.Description, Sensitive: b.Sensitive, - block: block, + Range: block.DefRange, } attrs, moreDiags := b.Rest.JustAttributes() From f0131642986f208d363d9187a06cef0d78414641 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:49:38 +0100 Subject: [PATCH 16/21] bup modules --- go.mod | 5 +---- go.sum | 6 ++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 400435df1..4378b8cb1 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,6 @@ require ( github.com/aliyun/aliyun-oss-go-sdk v0.0.0-20170113022742-e6dbea820a9f github.com/antchfx/htmlquery v1.0.0 // indirect github.com/antchfx/xmlquery v1.0.0 // indirect - github.com/antchfx/xpath v0.0.0-20170728053731-b5c552e1acbd // indirect github.com/antchfx/xquery v0.0.0-20170730121040-eb8c3c172607 // indirect github.com/approvals/go-approval-tests v0.0.0-20160714161514-ad96e53bea43 @@ -153,7 +152,7 @@ require ( github.com/xanzy/go-cloudstack v0.0.0-20190526095453-42f262b63ed0 github.com/yandex-cloud/go-genproto v0.0.0-20190916101622-7617782d381e github.com/yandex-cloud/go-sdk v0.0.0-20190916101744-c781afa45829 - github.com/zclconf/go-cty v1.2.1 + github.com/zclconf/go-cty v1.3.1 github.com/zclconf/go-cty-yaml v1.0.1 go.opencensus.io v0.22.2 // indirect golang.org/x/crypto v0.0.0-20200117160349-530e935923ad @@ -180,6 +179,4 @@ replace git.apache.org/thrift.git => github.com/apache/thrift v0.0.0-20180902110 replace github.com/gofrs/flock => github.com/azr/flock v0.0.0-20190823144736-958d66434653 -replace github.com/zclconf/go-cty => github.com/azr/go-cty v1.1.1-0.20200203143058-28fcda2fe0cc - go 1.13 diff --git a/go.sum b/go.sum index 36a39c483..85d9d39b8 100644 --- a/go.sum +++ b/go.sum @@ -488,6 +488,11 @@ github.com/yandex-cloud/go-genproto v0.0.0-20190916101622-7617782d381e h1:hzwq5G github.com/yandex-cloud/go-genproto v0.0.0-20190916101622-7617782d381e/go.mod h1:HEUYX/p8966tMUHHT+TsS0hF/Ca/NYwqprC5WXSDMfE= github.com/yandex-cloud/go-sdk v0.0.0-20190916101744-c781afa45829 h1:2FGwbx03GpP1Ulzg/L46tSoKh9t4yg8BhMKQl/Ff1x8= github.com/yandex-cloud/go-sdk v0.0.0-20190916101744-c781afa45829/go.mod h1:Eml0jFLU4VVHgIN8zPHMuNwZXVzUMILyO6lQZSfz854= +github.com/zclconf/go-cty v1.0.0/go.mod h1:xnAOWiHeOqg2nWS62VtQ7pbOu17FtxJNW8RLEih+O3s= +github.com/zclconf/go-cty v1.2.0/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= +github.com/zclconf/go-cty v1.2.1/go.mod h1:hOPWgoHbaTUnI5k4D2ld+GRpFJSCe6bCM7m1q/N4PQ8= +github.com/zclconf/go-cty v1.3.1 h1:QIOZl+CKKdkv4l2w3lG23nNzXgLoxsWLSEdg1MlX4p0= +github.com/zclconf/go-cty v1.3.1/go.mod h1:YO23e2L18AG+ZYQfSobnY4G65nvwvprPCxBHkufUH1k= github.com/zclconf/go-cty-yaml v1.0.1 h1:up11wlgAaDvlAGENcFDnZgkn0qUJurso7k6EpURKNF8= github.com/zclconf/go-cty-yaml v1.0.1/go.mod h1:IP3Ylp0wQpYm50IHK8OZWKMu6sPJIUgKa8XhiVHura0= go.opencensus.io v0.21.0 h1:mU6zScU4U1YAFPHEHYk+3JC4SY7JxgkqS10ZOSyksNg= @@ -541,6 +546,7 @@ golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zH golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg= golang.org/x/net v0.0.0-20180218175443-cbe0f9307d01/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= +golang.org/x/net v0.0.0-20180811021610-c39426892332/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20180826012351-8a410e7b638d/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181023162649-9b4f9f5ad519/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20181114220301-adae6a3d119a/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= From e195c627fbaf8899dd97b2bb0f440a9a53635c3b Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Mar 2020 13:50:15 +0100 Subject: [PATCH 17/21] vendor vendors --- .../zclconf/go-cty/cty/convert/conversion.go | 9 + .../cty/convert/conversion_collection.go | 186 ++++++++++++++++-- .../zclconf/go-cty/cty/convert/unify.go | 43 ++++ .../go-cty/cty/function/stdlib/collection.go | 182 ++++++++++++----- .../go-cty/cty/function/stdlib/datetime.go | 2 +- vendor/modules.txt | 2 +- 6 files changed, 359 insertions(+), 65 deletions(-) diff --git a/vendor/github.com/zclconf/go-cty/cty/convert/conversion.go b/vendor/github.com/zclconf/go-cty/cty/convert/conversion.go index ee35e9de7..8d177f151 100644 --- a/vendor/github.com/zclconf/go-cty/cty/convert/conversion.go +++ b/vendor/github.com/zclconf/go-cty/cty/convert/conversion.go @@ -138,6 +138,15 @@ func getConversionKnown(in cty.Type, out cty.Type, unsafe bool) conversion { outEty := out.ElementType() return conversionObjectToMap(in, outEty, unsafe) + case out.IsObjectType() && in.IsMapType(): + if !unsafe { + // Converting a map to an object is an "unsafe" conversion, + // because we don't know if all the map keys will correspond to + // object attributes. + return nil + } + return conversionMapToObject(in, out, unsafe) + case in.IsCapsuleType() || out.IsCapsuleType(): if !unsafe { // Capsule types can only participate in "unsafe" conversions, diff --git a/vendor/github.com/zclconf/go-cty/cty/convert/conversion_collection.go b/vendor/github.com/zclconf/go-cty/cty/convert/conversion_collection.go index 3039ba22e..ea23bf618 100644 --- a/vendor/github.com/zclconf/go-cty/cty/convert/conversion_collection.go +++ b/vendor/github.com/zclconf/go-cty/cty/convert/conversion_collection.go @@ -15,18 +15,18 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, val.LengthInt()) i := int64(0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -37,6 +37,9 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.ListValEmpty(ety), nil } @@ -55,18 +58,18 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, val.LengthInt()) i := int64(0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -77,6 +80,11 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + // Prefer a concrete type over a dynamic type when returning an + // empty set + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.SetValEmpty(ety), nil } @@ -93,13 +101,13 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion { func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make(map[string]cty.Value, 0) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { key, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: key, } @@ -107,11 +115,11 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { if err != nil { // Should never happen, because keys can only be numbers or // strings and both can convert to string. - return cty.DynamicVal, path.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName()) + return cty.DynamicVal, elemPath.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName()) } if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -121,9 +129,25 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion { } if len(elems) == 0 { + // Prefer a concrete type over a dynamic type when returning an + // empty map + if ety == cty.DynamicPseudoType { + ety = val.Type().ElementType() + } return cty.MapValEmpty(ety), nil } + if ety.IsCollectionType() || ety.IsObjectType() { + var err error + if elems, err = conversionUnifyCollectionElements(elems, path, false); err != nil { + return cty.NilVal, err + } + } + + if err := conversionCheckMapElementTypes(elems, path); err != nil { + return cty.NilVal, err + } + return cty.MapVal(elems), nil } } @@ -171,20 +195,20 @@ func conversionTupleToSet(tupleType cty.Type, listEty cty.Type, unsafe bool) con // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) i := int64(0) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } conv := elemConvs[i] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -241,20 +265,20 @@ func conversionTupleToList(tupleType cty.Type, listEty cty.Type, unsafe bool) co // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make([]cty.Value, 0, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) i := int64(0) it := val.ElementIterator() for it.Next() { _, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: cty.NumberIntVal(i), } conv := elemConvs[i] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -315,19 +339,19 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co // element conversions in elemConvs return func(val cty.Value, path cty.Path) (cty.Value, error) { elems := make(map[string]cty.Value, len(elemConvs)) - path = append(path, nil) + elemPath := append(path.Copy(), nil) it := val.ElementIterator() for it.Next() { name, val := it.Element() var err error - path[len(path)-1] = cty.IndexStep{ + elemPath[len(elemPath)-1] = cty.IndexStep{ Key: name, } conv := elemConvs[name.AsString()] if conv != nil { - val, err = conv(val, path) + val, err = conv(val, elemPath) if err != nil { return cty.NilVal, err } @@ -335,6 +359,130 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co elems[name.AsString()] = val } + if mapEty.IsCollectionType() || mapEty.IsObjectType() { + var err error + if elems, err = conversionUnifyCollectionElements(elems, path, unsafe); err != nil { + return cty.NilVal, err + } + } + + if err := conversionCheckMapElementTypes(elems, path); err != nil { + return cty.NilVal, err + } + return cty.MapVal(elems), nil } } + +// conversionMapToObject returns a conversion that will take a value of the +// given map type and return an object of the given type. The object attribute +// types must all be compatible with the map element type. +// +// Will panic if the given mapType and objType are not maps and objects +// respectively. +func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conversion { + objectAtys := objType.AttributeTypes() + mapEty := mapType.ElementType() + + elemConvs := make(map[string]conversion, len(objectAtys)) + for name, objectAty := range objectAtys { + if objectAty.Equals(mapEty) { + // no conversion required + continue + } + + elemConvs[name] = getConversion(mapEty, objectAty, unsafe) + if elemConvs[name] == nil { + // If any of our element conversions are impossible, then the our + // whole conversion is impossible. + return nil + } + } + + // If we fall out here then a conversion is possible, using the + // element conversions in elemConvs + return func(val cty.Value, path cty.Path) (cty.Value, error) { + elems := make(map[string]cty.Value, len(elemConvs)) + elemPath := append(path.Copy(), nil) + it := val.ElementIterator() + for it.Next() { + name, val := it.Element() + + // if there is no corresponding attribute, we skip this key + if _, ok := objectAtys[name.AsString()]; !ok { + continue + } + + var err error + + elemPath[len(elemPath)-1] = cty.IndexStep{ + Key: name, + } + + conv := elemConvs[name.AsString()] + if conv != nil { + val, err = conv(val, elemPath) + if err != nil { + return cty.NilVal, err + } + } + + elems[name.AsString()] = val + } + + return cty.ObjectVal(elems), nil + } +} + +func conversionUnifyCollectionElements(elems map[string]cty.Value, path cty.Path, unsafe bool) (map[string]cty.Value, error) { + elemTypes := make([]cty.Type, 0, len(elems)) + for _, elem := range elems { + elemTypes = append(elemTypes, elem.Type()) + } + unifiedType, _ := unify(elemTypes, unsafe) + if unifiedType == cty.NilType { + } + + unifiedElems := make(map[string]cty.Value) + elemPath := append(path.Copy(), nil) + + for name, elem := range elems { + if elem.Type().Equals(unifiedType) { + unifiedElems[name] = elem + continue + } + conv := getConversion(elem.Type(), unifiedType, unsafe) + if conv == nil { + } + elemPath[len(elemPath)-1] = cty.IndexStep{ + Key: cty.StringVal(name), + } + val, err := conv(elem, elemPath) + if err != nil { + return nil, err + } + unifiedElems[name] = val + } + + return unifiedElems, nil +} + +func conversionCheckMapElementTypes(elems map[string]cty.Value, path cty.Path) error { + elementType := cty.NilType + elemPath := append(path.Copy(), nil) + + for name, elem := range elems { + if elementType == cty.NilType { + elementType = elem.Type() + continue + } + if !elementType.Equals(elem.Type()) { + elemPath[len(elemPath)-1] = cty.IndexStep{ + Key: cty.StringVal(name), + } + return elemPath.NewErrorf("%s is required", elementType.FriendlyName()) + } + } + + return nil +} diff --git a/vendor/github.com/zclconf/go-cty/cty/convert/unify.go b/vendor/github.com/zclconf/go-cty/cty/convert/unify.go index 53ebbfe08..ee171d1ed 100644 --- a/vendor/github.com/zclconf/go-cty/cty/convert/unify.go +++ b/vendor/github.com/zclconf/go-cty/cty/convert/unify.go @@ -28,11 +28,14 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) { // a subset of that type, which would be a much less useful conversion for // unification purposes. { + mapCt := 0 objectCt := 0 tupleCt := 0 dynamicCt := 0 for _, ty := range types { switch { + case ty.IsMapType(): + mapCt++ case ty.IsObjectType(): objectCt++ case ty.IsTupleType(): @@ -44,6 +47,8 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) { } } switch { + case mapCt > 0 && (mapCt+dynamicCt) == len(types): + return unifyMapTypes(types, unsafe, dynamicCt > 0) case objectCt > 0 && (objectCt+dynamicCt) == len(types): return unifyObjectTypes(types, unsafe, dynamicCt > 0) case tupleCt > 0 && (tupleCt+dynamicCt) == len(types): @@ -95,6 +100,44 @@ Preferences: return cty.NilType, nil } +func unifyMapTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) { + // If we had any dynamic types in the input here then we can't predict + // what path we'll take through here once these become known types, so + // we'll conservatively produce DynamicVal for these. + if hasDynamic { + return unifyAllAsDynamic(types) + } + + elemTypes := make([]cty.Type, 0, len(types)) + for _, ty := range types { + elemTypes = append(elemTypes, ty.ElementType()) + } + retElemType, _ := unify(elemTypes, unsafe) + if retElemType == cty.NilType { + return cty.NilType, nil + } + + retTy := cty.Map(retElemType) + + conversions := make([]Conversion, len(types)) + for i, ty := range types { + if ty.Equals(retTy) { + continue + } + if unsafe { + conversions[i] = GetConversionUnsafe(ty, retTy) + } else { + conversions[i] = GetConversion(ty, retTy) + } + if conversions[i] == nil { + // Shouldn't be reachable, since we were able to unify + return cty.NilType, nil + } + } + + return retTy, conversions +} + func unifyObjectTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) { // If we had any dynamic types in the input here then we can't predict // what path we'll take through here once these become known types, so diff --git a/vendor/github.com/zclconf/go-cty/cty/function/stdlib/collection.go b/vendor/github.com/zclconf/go-cty/cty/function/stdlib/collection.go index 4b96f9dd3..b2ce062a6 100644 --- a/vendor/github.com/zclconf/go-cty/cty/function/stdlib/collection.go +++ b/vendor/github.com/zclconf/go-cty/cty/function/stdlib/collection.go @@ -299,7 +299,7 @@ var ContainsFunc = function.New(&function.Spec{ }, }, Type: function.StaticReturnType(cty.Bool), - Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { arg := args[0] ty := arg.Type() @@ -307,12 +307,39 @@ var ContainsFunc = function.New(&function.Spec{ return cty.NilVal, errors.New("argument must be list, tuple, or set") } - _, err = Index(cty.TupleVal(arg.AsValueSlice()), args[1]) - if err != nil { + if args[0].IsNull() { + return cty.NilVal, errors.New("cannot search a nil list or set") + } + + if args[0].LengthInt() == 0 { return cty.False, nil } - return cty.True, nil + if !args[0].IsKnown() || !args[1].IsKnown() { + return cty.UnknownVal(cty.Bool), nil + } + + containsUnknown := false + for it := args[0].ElementIterator(); it.Next(); { + _, v := it.Element() + eq := args[1].Equals(v) + if !eq.IsKnown() { + // We may have an unknown value which could match later, but we + // first need to continue checking all values for an exact + // match. + containsUnknown = true + continue + } + if eq.True() { + return cty.True, nil + } + } + + if containsUnknown { + return cty.UnknownVal(cty.Bool), nil + } + + return cty.False, nil }, }) @@ -566,19 +593,12 @@ var LookupFunc = function.New(&function.Spec{ Name: "key", Type: cty.String, }, - }, - VarParam: &function.Parameter{ - Name: "default", - Type: cty.DynamicPseudoType, - AllowUnknown: true, - AllowDynamicType: true, - AllowNull: true, + { + Name: "default", + Type: cty.DynamicPseudoType, + }, }, Type: func(args []cty.Value) (ret cty.Type, err error) { - if len(args) < 1 || len(args) > 3 { - return cty.NilType, fmt.Errorf("lookup() takes two or three arguments, got %d", len(args)) - } - ty := args[0].Type() switch { @@ -609,13 +629,7 @@ var LookupFunc = function.New(&function.Spec{ } }, Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { - var defaultVal cty.Value - defaultValueSet := false - - if len(args) == 3 { - defaultVal = args[2] - defaultValueSet = true - } + defaultVal := args[2] mapVar := args[0] lookupKey := args[1].AsString() @@ -632,48 +646,128 @@ var LookupFunc = function.New(&function.Spec{ return mapVar.Index(cty.StringVal(lookupKey)), nil } - if defaultValueSet { - defaultVal, err = convert.Convert(defaultVal, retType) - if err != nil { - return cty.NilVal, err - } - return defaultVal, nil + defaultVal, err = convert.Convert(defaultVal, retType) + if err != nil { + return cty.NilVal, err } - - return cty.UnknownVal(cty.DynamicPseudoType), fmt.Errorf( - "lookup failed to find '%s'", lookupKey) + return defaultVal, nil }, }) -// MergeFunc is a function that takes an arbitrary number of maps and -// returns a single map that contains a merged set of elements from all of the maps. +// MergeFunc constructs a function that takes an arbitrary number of maps or +// objects, and returns a single value that contains a merged set of keys and +// values from all of the inputs. // -// If more than one given map defines the same key then the one that is later in -// the argument sequence takes precedence. +// If more than one given map or object defines the same key then the one that +// is later in the argument sequence takes precedence. var MergeFunc = function.New(&function.Spec{ Params: []function.Parameter{}, VarParam: &function.Parameter{ Name: "maps", Type: cty.DynamicPseudoType, AllowDynamicType: true, + AllowNull: true, + }, + Type: func(args []cty.Value) (cty.Type, error) { + // empty args is accepted, so assume an empty object since we have no + // key-value types. + if len(args) == 0 { + return cty.EmptyObject, nil + } + + // collect the possible object attrs + attrs := map[string]cty.Type{} + + first := cty.NilType + matching := true + attrsKnown := true + for i, arg := range args { + ty := arg.Type() + // any dynamic args mean we can't compute a type + if ty.Equals(cty.DynamicPseudoType) { + return cty.DynamicPseudoType, nil + } + + // check for invalid arguments + if !ty.IsMapType() && !ty.IsObjectType() { + return cty.NilType, fmt.Errorf("arguments must be maps or objects, got %#v", ty.FriendlyName()) + } + + switch { + case ty.IsObjectType() && !arg.IsNull(): + for attr, aty := range ty.AttributeTypes() { + attrs[attr] = aty + } + case ty.IsMapType(): + switch { + case arg.IsNull(): + // pass, nothing to add + case arg.IsKnown(): + ety := arg.Type().ElementType() + for it := arg.ElementIterator(); it.Next(); { + attr, _ := it.Element() + attrs[attr.AsString()] = ety + } + default: + // any unknown maps means we don't know all possible attrs + // for the return type + attrsKnown = false + } + } + + // record the first argument type for comparison + if i == 0 { + first = arg.Type() + continue + } + + if !ty.Equals(first) && matching { + matching = false + } + } + + // the types all match, so use the first argument type + if matching { + return first, nil + } + + // We had a mix of unknown maps and objects, so we can't predict the + // attributes + if !attrsKnown { + return cty.DynamicPseudoType, nil + } + + return cty.Object(attrs), nil }, - Type: function.StaticReturnType(cty.DynamicPseudoType), Impl: func(args []cty.Value, retType cty.Type) (ret cty.Value, err error) { outputMap := make(map[string]cty.Value) + // if all inputs are null, return a null value rather than an object + // with null attributes + allNull := true for _, arg := range args { - if !arg.IsWhollyKnown() { - return cty.UnknownVal(retType), nil - } - if !arg.Type().IsObjectType() && !arg.Type().IsMapType() { - return cty.NilVal, fmt.Errorf("arguments must be maps or objects, got %#v", arg.Type().FriendlyName()) + if arg.IsNull() { + continue + } else { + allNull = false } + for it := arg.ElementIterator(); it.Next(); { k, v := it.Element() outputMap[k.AsString()] = v } } - return cty.ObjectVal(outputMap), nil + + switch { + case allNull: + return cty.NullVal(retType), nil + case retType.IsMapType(): + return cty.MapVal(outputMap), nil + case retType.IsObjectType(), retType.Equals(cty.DynamicPseudoType): + return cty.ObjectVal(outputMap), nil + default: + panic(fmt.Sprintf("unexpected return type: %#v", retType)) + } }, }) @@ -1184,8 +1278,8 @@ func Keys(inputMap cty.Value) (cty.Value, error) { // Lookup performs a dynamic lookup into a map. // There are two required arguments, map and key, plus an optional default, // which is a value to return if no key is found in map. -func Lookup(args ...cty.Value) (cty.Value, error) { - return LookupFunc.Call(args) +func Lookup(inputMap, key, defaultValue cty.Value) (cty.Value, error) { + return LookupFunc.Call([]cty.Value{inputMap, key, defaultValue}) } // Merge takes an arbitrary number of maps and returns a single map that contains diff --git a/vendor/github.com/zclconf/go-cty/cty/function/stdlib/datetime.go b/vendor/github.com/zclconf/go-cty/cty/function/stdlib/datetime.go index a1aa5f4b5..3ce41ba9d 100644 --- a/vendor/github.com/zclconf/go-cty/cty/function/stdlib/datetime.go +++ b/vendor/github.com/zclconf/go-cty/cty/function/stdlib/datetime.go @@ -217,7 +217,7 @@ var TimeAddFunc = function.New(&function.Spec{ }, Type: function.StaticReturnType(cty.String), Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { - ts, err := time.Parse(time.RFC3339, args[0].AsString()) + ts, err := parseTimestamp(args[0].AsString()) if err != nil { return cty.UnknownVal(cty.String), err } diff --git a/vendor/modules.txt b/vendor/modules.txt index 65491cc03..12fae6eca 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -638,7 +638,7 @@ github.com/yandex-cloud/go-sdk/pkg/retry github.com/yandex-cloud/go-sdk/pkg/sdkerrors github.com/yandex-cloud/go-sdk/pkg/singleflight github.com/yandex-cloud/go-sdk/sdkresolvers -# github.com/zclconf/go-cty v1.2.1 => github.com/azr/go-cty v1.1.1-0.20200203143058-28fcda2fe0cc +# github.com/zclconf/go-cty v1.3.1 github.com/zclconf/go-cty/cty github.com/zclconf/go-cty/cty/convert github.com/zclconf/go-cty/cty/function From 6d8cce501e25efae057362eed8d7f1e4adae35e7 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 9 Mar 2020 16:16:59 +0100 Subject: [PATCH 18/21] tweak validation & add tests --- hcl2template/parser.go | 2 +- hcl2template/testdata/variables/empty.pkr.hcl | 0 hcl2template/types.packer_config.go | 6 ++ hcl2template/types.variables.go | 22 ++++- hcl2template/types.variables_test.go | 85 ++++++++++++++----- 5 files changed, 89 insertions(+), 26 deletions(-) create mode 100644 hcl2template/testdata/variables/empty.pkr.hcl diff --git a/hcl2template/parser.go b/hcl2template/parser.go index 9ecbde9a2..df6ef1d86 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -123,7 +123,7 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, varFiles = append(varFiles, f) } - diags = append(diags, cfg.InputVariables.collectVariableValues(os.Environ(), varFiles, vars)...) + diags = append(diags, cfg.collectInputVariableValues(os.Environ(), varFiles, vars)...) } // decode the actual content diff --git a/hcl2template/testdata/variables/empty.pkr.hcl b/hcl2template/testdata/variables/empty.pkr.hcl new file mode 100644 index 000000000..e69de29bb diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 5748237b2..d972dd90c 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -25,10 +25,16 @@ type PackerConfig struct { InputVariables Variables LocalVariables Variables + ValidationOptions + // Builds is the list of Build blocks defined in the config files. Builds Builds } +type ValidationOptions struct { + Strict bool +} + // EvalContext returns the *hcl.EvalContext that will be passed to an hcl // decoder in order to tell what is the actual value of a var or a local and // the list of defined functions. diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 55158379b..5dc5579c2 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -223,8 +223,9 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval // them. const VarEnvPrefix = "PKR_VAR_" -func (variables Variables) collectVariableValues(env []string, files []*hcl.File, argv map[string]string) hcl.Diagnostics { +func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.File, argv map[string]string) hcl.Diagnostics { var diags hcl.Diagnostics + variables := cfg.InputVariables for _, raw := range env { if !strings.HasPrefix(raw, VarEnvPrefix) { @@ -321,7 +322,20 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File for name, attr := range attrs { variable, found := variables[name] if !found { - // No file defines this variable; let's skip it + sev := hcl.DiagWarning + if cfg.ValidationOptions.Strict { + sev = hcl.DiagError + } + diags = append(diags, &hcl.Diagnostic{ + Severity: sev, + Summary: "Undefined variable", + Detail: fmt.Sprintf("A %q variable was set but was "+ + "not found in known variables. To declare "+ + "variable %q, place this block in one of your"+ + ".pkr files, such as variables.pkr.hcl", + name, name), + Context: attr.Range.Ptr(), + }) continue } @@ -351,8 +365,8 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File variable, found := variables[name] if !found { diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Unknown -var variable", + Severity: hcl.DiagError, + Summary: "Undefined -var variable", Detail: fmt.Sprintf("A %q variable was passed in the command "+ "line but was not found in known variables."+ "To declare variable %q, place this block in one of your"+ diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index b4cd03b78..7e9863284 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -247,12 +247,14 @@ func TestVariables_collectVariableValues(t *testing.T) { argv map[string]string } tests := []struct { - name string - variables Variables - args args - wantDiags bool - wantVariables Variables - wantValues map[string]cty.Value + name string + variables Variables + validationOptions ValidationOptions + args args + wantDiags bool + wantDiagsHasError bool + wantVariables Variables + wantValues map[string]cty.Value }{ {name: "string", @@ -333,11 +335,39 @@ func TestVariables_collectVariableValues(t *testing.T) { }, }, - {name: "undefined but set value", + {name: "undefined but set value - pkrvar file - normal mode", variables: Variables{}, args: args{ - env: []string{`PKR_VAR_unused_string=value`}, - hclFiles: []string{`unused_string="value"`}, + hclFiles: []string{`undefined_string="value"`}, + }, + + // output + wantDiags: true, + wantDiagsHasError: false, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, + }, + + {name: "undefined but set value - pkrvar file - strict mode", + variables: Variables{}, + validationOptions: ValidationOptions{ + Strict: true, + }, + args: args{ + hclFiles: []string{`undefined_string="value"`}, + }, + + // output + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, + }, + + {name: "undefined but set value - env", + variables: Variables{}, + args: args{ + env: []string{`PKR_VAR_undefined_string=value`}, }, // output @@ -346,18 +376,19 @@ func TestVariables_collectVariableValues(t *testing.T) { wantValues: map[string]cty.Value{}, }, - {name: "undefined but set value - args", + {name: "undefined but set value - argv", variables: Variables{}, args: args{ argv: map[string]string{ - "unused_string": "value", + "undefined_string": "value", }, }, // output - wantDiags: true, - wantVariables: Variables{}, - wantValues: map[string]cty.Value{}, + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, }, {name: "value not corresponding to type - env", @@ -371,7 +402,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -394,7 +426,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -419,7 +452,8 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, + wantDiags: true, + wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ Type: cty.String, @@ -438,9 +472,10 @@ func TestVariables_collectVariableValues(t *testing.T) { }, // output - wantDiags: true, - wantVariables: Variables{}, - wantValues: map[string]cty.Value{}, + wantDiags: true, + wantDiagsHasError: true, + wantVariables: Variables{}, + wantValues: map[string]cty.Value{}, }, } for _, tt := range tests { @@ -454,9 +489,17 @@ func TestVariables_collectVariableValues(t *testing.T) { } files = append(files, file) } - if gotDiags := tt.variables.collectVariableValues(tt.args.env, files, tt.args.argv); (gotDiags == nil) == tt.wantDiags { + cfg := &PackerConfig{ + InputVariables: tt.variables, + ValidationOptions: tt.validationOptions, + } + gotDiags := cfg.collectInputVariableValues(tt.args.env, files, tt.args.argv) + if (gotDiags == nil) == tt.wantDiags { t.Fatalf("Variables.collectVariableValues() = %v, want %v", gotDiags, tt.wantDiags) } + if tt.wantDiagsHasError != gotDiags.HasErrors() { + t.Fatalf("Variables.collectVariableValues() unexpected diagnostics HasErrors. %s", gotDiags) + } if diff := cmp.Diff(fmt.Sprintf("%#v", tt.wantVariables), fmt.Sprintf("%#v", tt.variables)); diff != "" { t.Fatalf("didn't get expected variables: %s", diff) } From 0ccff0d5b96c0fa2651fb80575b1ffbe941f984f Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 9 Mar 2020 17:25:56 +0100 Subject: [PATCH 19/21] all variables must have a value. A variable's default value can be set to null to force user to set it. --- hcl2template/parser.go | 5 +++++ hcl2template/testdata/variables/basic.pkr.hcl | 1 + .../testdata/variables/unknown_key.pkr.hcl | 3 ++- hcl2template/types.packer_config.go | 6 ++++-- hcl2template/types.variables.go | 21 +++++++++++-------- hcl2template/types.variables_test.go | 16 ++++++++------ 6 files changed, 34 insertions(+), 18 deletions(-) diff --git a/hcl2template/parser.go b/hcl2template/parser.go index df6ef1d86..3bf48294e 100644 --- a/hcl2template/parser.go +++ b/hcl2template/parser.go @@ -126,6 +126,11 @@ func (p *Parser) parse(filename string, vars map[string]string) (*PackerConfig, diags = append(diags, cfg.collectInputVariableValues(os.Environ(), varFiles, vars)...) } + _, moreDiags := cfg.InputVariables.Values() + diags = append(diags, moreDiags...) + _, moreDiags = cfg.LocalVariables.Values() + diags = append(diags, moreDiags...) + // decode the actual content for _, file := range files { diags = append(diags, p.decodeConfig(file, cfg)...) diff --git a/hcl2template/testdata/variables/basic.pkr.hcl b/hcl2template/testdata/variables/basic.pkr.hcl index c495ed741..8fb119b0c 100644 --- a/hcl2template/testdata/variables/basic.pkr.hcl +++ b/hcl2template/testdata/variables/basic.pkr.hcl @@ -29,6 +29,7 @@ variable "super_secret_password" { description = < Date: Tue, 10 Mar 2020 14:51:17 +0100 Subject: [PATCH 20/21] document variables behaviours with tables --- .../configuration/from-1.5/variables.html.md | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/website/source/docs/configuration/from-1.5/variables.html.md b/website/source/docs/configuration/from-1.5/variables.html.md index dda0c11d2..d43866814 100644 --- a/website/source/docs/configuration/from-1.5/variables.html.md +++ b/website/source/docs/configuration/from-1.5/variables.html.md @@ -278,3 +278,48 @@ precedence over earlier ones: ~> **Important:** Variables with map and object values behave the same way as other variables: the last value found overrides the previous values. + +### A variable value must be known : + +Take the following variable for example: + +``` hcl +variable "foo" { + type = string +``` + +Here `foo` must have a known value but you can default it to `null` to make +this behavior optional : + +| | no default | `default = null` | `default = "xy"` | +|:---------------------------:|:----------------------------:|:----------------:|:----------------:| +| foo unused | error, "foo needs to be set" | - | - | +| var.foo | error, "foo needs to be set" | null¹ | yz | +| `PKR_VAR_foo=yz`
var.foo | yz | yz | yz | +| `-var foo=yz`
var.foo | yz | yz | yz | + +1: Null is a valid value. Packer will only error when the receiving field needs +a value, example: + +``` hcl +variable "example" { + type = string + default = null +} + +source "example" "foo" { + arg = var.example +} +``` + +In the above case, as long as "arg" is optional for an "example" source, there is no error and arg won’t be set. + + +### Setting an unknown variable will not always fail : + +| Usage | packer validate | any other packer command | +|:------------------------------:|:-----------------------:|:-------------------------:| +| `bar=yz` in .pkrvars.hcl file. | error, "bar undeclared" | warning, "bar undeclared" | +| `var.bar` in .pkr.hcl file | error, "bar undeclared" | error, "bar undeclared" | +| `-var bar=yz` argument | error, "bar undeclared" | error, "bar undeclared" | +| `export PKR_VAR_bar=yz` | - | - | From d4d30127a3e0ab9d2d84879a9e3a426c433ab2ea Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 10 Mar 2020 14:53:48 +0100 Subject: [PATCH 21/21] Update variables.html.md --- website/source/docs/configuration/from-1.5/variables.html.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/source/docs/configuration/from-1.5/variables.html.md b/website/source/docs/configuration/from-1.5/variables.html.md index d43866814..809383226 100644 --- a/website/source/docs/configuration/from-1.5/variables.html.md +++ b/website/source/docs/configuration/from-1.5/variables.html.md @@ -294,7 +294,7 @@ this behavior optional : | | no default | `default = null` | `default = "xy"` | |:---------------------------:|:----------------------------:|:----------------:|:----------------:| | foo unused | error, "foo needs to be set" | - | - | -| var.foo | error, "foo needs to be set" | null¹ | yz | +| var.foo | error, "foo needs to be set" | null¹ | xy | | `PKR_VAR_foo=yz`
var.foo | yz | yz | yz | | `-var foo=yz`
var.foo | yz | yz | yz |