From 0ccff0d5b96c0fa2651fb80575b1ffbe941f984f Mon Sep 17 00:00:00 2001
From: Adrien Delorme <adrien.delorme@icloud.com>
Date: Mon, 9 Mar 2020 17:25:56 +0100
Subject: [PATCH] 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 = <<IMSENSIBLE
 Handle with care plz
 IMSENSIBLE
+  default = null
 }
 
 locals {
diff --git a/hcl2template/testdata/variables/unknown_key.pkr.hcl b/hcl2template/testdata/variables/unknown_key.pkr.hcl
index c04d5e497..e5db1ead9 100644
--- a/hcl2template/testdata/variables/unknown_key.pkr.hcl
+++ b/hcl2template/testdata/variables/unknown_key.pkr.hcl
@@ -1,4 +1,5 @@
 
-variable "broken_type" {
+variable "broken_variable" {
     invalid = true
+    default = true
 }
diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go
index d972dd90c..0d61e3069 100644
--- a/hcl2template/types.packer_config.go
+++ b/hcl2template/types.packer_config.go
@@ -39,11 +39,13 @@ type ValidationOptions 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 {
+	inputVariables, _ := cfg.InputVariables.Values()
+	localVariables, _ := 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(inputVariables),
+			"local": cty.ObjectVal(localVariables),
 		},
 	}
 	return ectx
diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go
index 5dc5579c2..a9af6ca58 100644
--- a/hcl2template/types.variables.go
+++ b/hcl2template/types.variables.go
@@ -62,7 +62,7 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) {
 		v.EnvValue,
 		v.DefaultValue,
 	} {
-		if !value.IsNull() {
+		if value != cty.NilVal {
 			return value, nil
 		}
 	}
@@ -73,23 +73,26 @@ func (v *Variable) Value() (cty.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 " +
-			"https://packer.io/docs/configuration/from-1.5/syntax.html for details.",
+			"https://packer.io/docs/configuration/from-1.5/syntax.html for " +
+			"details.",
 		Context: v.Range.Ptr(),
 	}
 }
 
 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()
-		// 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.
+		value, diag := v.Value()
+		if diag != nil {
+			diags = append(diags, diag)
+			continue
+		}
+		res[k] = value
 	}
-	return res
+	return res, diags
 }
 
 // decodeVariable decodes a variable key and value into Variables
diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go
index 7e9863284..c9e942496 100644
--- a/hcl2template/types.variables_test.go
+++ b/hcl2template/types.variables_test.go
@@ -52,9 +52,10 @@ func TestParse_variables(t *testing.T) {
 						Description: fmt.Sprintln("Describing is awesome ;D"),
 					},
 					"super_secret_password": &Variable{
-						Name:        "super_secret_password",
-						Sensitive:   true,
-						Description: fmt.Sprintln("Handle with care plz"),
+						Name:         "super_secret_password",
+						Sensitive:    true,
+						DefaultValue: cty.NullVal(cty.String),
+						Description:  fmt.Sprintln("Handle with care plz"),
 					},
 				},
 				LocalVariables: Variables{
@@ -117,14 +118,16 @@ func TestParse_variables(t *testing.T) {
 			[]packer.Build{},
 			false,
 		},
+
 		{"unknown key",
 			defaultParser,
 			parseTestArgs{"testdata/variables/unknown_key.pkr.hcl", nil},
 			&PackerConfig{
 				Basedir: filepath.Join("testdata", "variables"),
 				InputVariables: Variables{
-					"broken_type": &Variable{
-						Name: "broken_type",
+					"broken_variable": &Variable{
+						Name:         "broken_variable",
+						DefaultValue: cty.BoolVal(true),
 					},
 				},
 			},
@@ -132,6 +135,7 @@ func TestParse_variables(t *testing.T) {
 			[]packer.Build{},
 			false,
 		},
+
 		{"unset used variable",
 			defaultParser,
 			parseTestArgs{"testdata/variables/unset_used_string_variable.pkr.hcl", nil},
@@ -170,7 +174,7 @@ func TestParse_variables(t *testing.T) {
 					},
 				},
 			},
-			false, false,
+			true, true,
 			[]packer.Build{
 				&packer.CoreBuild{
 					Type:           "null",