hcl2template/types.variables: Update logic for parsing literal value variables (#8834)

* hcl2template/types.variables: Update logic for parsing literal value variables

In running tests via the CLI it was determined that when using the variable block with no explicit type assigned the type of the default
value was not being set within the map. This change updates the `decodeConfig` method so that a type is always set for any defined variable if not specified.

The second change is to properly handle the evaluation of basic variable types (e.g String, Number, Bool). Previously variables defined on the
CLI or via PKR_VAR required some additional quoting to for proper evaluation. This change fixes that issue so that it works like it does in Terraform :)

Build results before the change
```
⇶  PKR_VAR_example='["one","two"]' ~/bin/packer build -var 'foo=home' .
Error: Variables not allowed

  on <value for var.foo from arguments> line 1:
  (source code not available)

Variables may not be used here.

==> Builds finished but no artifacts were created.

```

Build results after the change
```
⇶  PKR_VAR_example='["one","two"]' ~/bin/packer build -var 'foo="home"' .
null: output will be in this color.

==> null: Running local shell script: /tmp/packer-shell885249462
    null: two
    null: home
Build 'null' finished.

==> Builds finished. The artifacts of successful builds are:
--> null: Did not export anything. This is the null builder

⇶  ~/bin/packer build -var 'foo=home' -var 'example=["one","another variable"]' .
null: output will be in this color.

==> null: Running local shell script: /tmp/packer-shell123467506
    null: another variable
    null: home
Build 'null' finished.

==> Builds finished. The artifacts of successful builds are:
--> null: Did not export anything. This is the null builder

```

* tests/hcl2template/types.variables: Update test to use Bool

Turns out a string value won't actually complain if it's given a non
string looking value. It will just covert the value to a string literal
so using a type Bool which should fail if given anything that is not
true or false.

* tests/hcl2template/types.variables: Update unit tests

During testing it was found that by default the variable stanza were defaulting to a cty.NilType, and not the Type of it's default value.
This change sets the default type of the defined variable to ensure variable evaluation behaviors correctly.

* Add a simple cty.Bool test case

* tests/hcl2template/types.variables:  Enable quoted_string test case

* Update hcl2template/types.variables.go

space

Co-authored-by: Adrien Delorme <azr@users.noreply.github.com>

hcl2template/types.variables.go

@@ -160,7 +160,6 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval
 		}
 
 		res.Type = tp
-		delete(attrs, "type")
 	}
 
 	if def, ok := attrs["default"]; ok {
@@ -186,6 +185,13 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval
 		}
 
 		res.DefaultValue = defaultValue
+
+		// It's possible no type attribute was assigned so lets make
+		// sure we have a valid type otherwise there will be issues parsing the value.
+		if res.Type == cty.NilType {
+			res.Type = res.DefaultValue.Type()
+		}
+
 	}
 	if len(attrs) > 0 {
 		keys := []string{}
@@ -234,14 +240,13 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File
 		}
 
 		fakeFilename := fmt.Sprintf("<value for var.%s from env>", name)
-		expr, moreDiags := hclsyntax.ParseExpression([]byte(value), fakeFilename, hcl.Pos{Line: 1, Column: 1})
+		expr, moreDiags := expressionFromVariableDefinition(fakeFilename, value, variable.Type)
 		diags = append(diags, moreDiags...)
 		if moreDiags.HasErrors() {
 			continue
 		}
 		val, valDiags := expr.Value(nil)
 		diags = append(diags, valDiags...)
-
 		if variable.Type != cty.NilType {
 			var err error
 			val, err = convert.Convert(val, variable.Type)
@@ -255,7 +260,6 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File
 				val = cty.DynamicVal
 			}
 		}
-
 		variable.EnvValue = val
 	}
 
@@ -348,11 +352,12 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File
 		}
 
 		fakeFilename := fmt.Sprintf("<value for var.%s from arguments>", name)
-		expr, moreDiags := hclsyntax.ParseExpression([]byte(value), fakeFilename, hcl.Pos{Line: 1, Column: 1})
+		expr, moreDiags := expressionFromVariableDefinition(fakeFilename, value, variable.Type)
 		diags = append(diags, moreDiags...)
 		if moreDiags.HasErrors() {
 			continue
 		}
+
 		val, valDiags := expr.Value(nil)
 		diags = append(diags, valDiags...)
 
@@ -375,3 +380,14 @@ func (variables Variables) collectVariableValues(env []string, files []*hcl.File
 
 	return diags
 }
+
+// expressionFromVariableDefinition creates an hclsyntax.Expression that is capable of evaluating the specified value for a given cty.Type.
+// The specified filename is to identify the source of where value originated from in the diagnostics report, if there is an error.
+func expressionFromVariableDefinition(filename string, value string, variableType cty.Type) (hclsyntax.Expression, hcl.Diagnostics) {
+	switch variableType {
+	case cty.String, cty.Number:
+		return &hclsyntax.LiteralValueExpr{Val: cty.StringVal(value)}, nil
+	default:
+		return hclsyntax.ParseExpression([]byte(value), filename, hcl.Pos{Line: 1, Column: 1})
+	}
+}

hcl2template/types.variables_test.go

@@ -181,15 +181,18 @@ func TestVariables_collectVariableValues(t *testing.T) {
 	}{
 
 		{name: "string",
-			variables: Variables{"used_string": &Variable{DefaultValue: cty.StringVal("default_value")}},
+			variables: Variables{"used_string": &Variable{
+				DefaultValue: cty.StringVal("default_value"),
+				Type:         cty.String,
+			}},
 			args: args{
-				env: []string{`PKR_VAR_used_string="env_value"`},
+				env: []string{`PKR_VAR_used_string=env_value`},
 				hclFiles: []string{
 					`used_string="xy"`,
 					`used_string="varfile_value"`,
 				},
 				argv: map[string]string{
-					"used_string": `"cmd_value"`,
+					"used_string": `cmd_value`,
 				},
 			},
 
@@ -197,6 +200,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			wantDiags: false,
 			wantVariables: Variables{
 				"used_string": &Variable{
+					Type:         cty.String,
 					CmdValue:     cty.StringVal("cmd_value"),
 					VarfileValue: cty.StringVal("varfile_value"),
 					EnvValue:     cty.StringVal("env_value"),
@@ -208,6 +212,38 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			},
 		},
 
+		{name: "quoted string",
+			variables: Variables{"quoted_string": &Variable{
+				DefaultValue: cty.StringVal(`"default_value"`),
+				Type:         cty.String,
+			}},
+			args: args{
+				env: []string{`PKR_VAR_quoted_string="env_value"`},
+				hclFiles: []string{
+					`quoted_string="\"xy\""`,
+					`quoted_string="\"varfile_value\""`,
+				},
+				argv: map[string]string{
+					"quoted_string": `"cmd_value"`,
+				},
+			},
+
+			// output
+			wantDiags: false,
+			wantVariables: Variables{
+				"quoted_string": &Variable{
+					Type:         cty.String,
+					CmdValue:     cty.StringVal(`"cmd_value"`),
+					VarfileValue: cty.StringVal(`"varfile_value"`),
+					EnvValue:     cty.StringVal(`"env_value"`),
+					DefaultValue: cty.StringVal(`"default_value"`),
+				},
+			},
+			wantValues: map[string]cty.Value{
+				"quoted_string": cty.StringVal(`"cmd_value"`),
+			},
+		},
+
 		{name: "array of strings",
 			variables: Variables{"used_strings": &Variable{
 				DefaultValue: stringListVal("default_value_1"),
@@ -240,8 +276,42 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			},
 		},
 
+		{name: "bool",
+			variables: Variables{"enabled": &Variable{
+				DefaultValue: cty.False,
+				Type:         cty.Bool,
+			}},
+			args: args{
+				env: []string{`PKR_VAR_enabled=true`},
+				hclFiles: []string{
+					`enabled="false"`,
+				},
+				argv: map[string]string{
+					"enabled": `true`,
+				},
+			},
+
+			// output
+			wantDiags: false,
+			wantVariables: Variables{
+				"enabled": &Variable{
+					Type:         cty.Bool,
+					CmdValue:     cty.True,
+					VarfileValue: cty.False,
+					EnvValue:     cty.True,
+					DefaultValue: cty.False,
+				},
+			},
+			wantValues: map[string]cty.Value{
+				"enabled": cty.True,
+			},
+		},
+
 		{name: "invalid env var",
-			variables: Variables{"used_string": &Variable{DefaultValue: cty.StringVal("default_value")}},
+			variables: Variables{"used_string": &Variable{
+				DefaultValue: cty.StringVal("default_value"),
+				Type:         cty.String,
+			}},
 			args: args{
 				env: []string{`PKR_VAR_used_string`},
 			},
@@ -250,6 +320,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			wantDiags: false,
 			wantVariables: Variables{
 				"used_string": &Variable{
+					Type:         cty.String,
 					DefaultValue: cty.StringVal("default_value"),
 				},
 			},
@@ -288,18 +359,18 @@ func TestVariables_collectVariableValues(t *testing.T) {
 		{name: "value not corresponding to type - env",
 			variables: Variables{
 				"used_string": &Variable{
-					Type: cty.String,
+					Type: cty.List(cty.String),
 				},
 			},
 			args: args{
-				env: []string{`PKR_VAR_used_string=["string"]`},
+				env: []string{`PKR_VAR_used_string="string"`},
 			},
 
 			// output
 			wantDiags: true,
 			wantVariables: Variables{
 				"used_string": &Variable{
-					Type:     cty.String,
+					Type:     cty.List(cty.String),
 					EnvValue: cty.DynamicVal,
 				},
 			},
@@ -311,7 +382,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
 		{name: "value not corresponding to type - cfg file",
 			variables: Variables{
 				"used_string": &Variable{
-					Type: cty.String,
+					Type: cty.Bool,
 				},
 			},
 			args: args{
@@ -322,7 +393,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			wantDiags: true,
 			wantVariables: Variables{
 				"used_string": &Variable{
-					Type:         cty.String,
+					Type:         cty.Bool,
 					VarfileValue: cty.DynamicVal,
 				},
 			},
@@ -334,12 +405,12 @@ func TestVariables_collectVariableValues(t *testing.T) {
 		{name: "value not corresponding to type - argv",
 			variables: Variables{
 				"used_string": &Variable{
-					Type: cty.String,
+					Type: cty.Bool,
 				},
 			},
 			args: args{
 				argv: map[string]string{
-					"used_string": `["string"]`,
+					"used_string": `["true"]`,
 				},
 			},
 
@@ -347,7 +418,7 @@ func TestVariables_collectVariableValues(t *testing.T) {
 			wantDiags: true,
 			wantVariables: Variables{
 				"used_string": &Variable{
-					Type:     cty.String,
+					Type:     cty.Bool,
 					CmdValue: cty.DynamicVal,
 				},
 			},