From 8d6b8da9963df4169b1cbbcdcbb27bf19e412386 Mon Sep 17 00:00:00 2001
From: Lucas Bajolet <lucas.bajolet@hashicorp.com>
Date: Thu, 5 Sep 2024 14:31:42 -0400
Subject: [PATCH] hcl2template: locals evaluation returns a variable

Evaluating local variables used to be directly written to the
PackerConfig while each variable was created.

This was somewhat of an issue with testing, as we have a bunch of tests
that relied on `PackerConfig.Variables` being set only when we actually
write something.

This is not really a concern for normal use, just for testing, but to
limit the number of changes to the tests in hcl2template, I opted to
change how variables' values are retained, so that evaluating a single
variable returns a Variable in addition to hcl.Diagnostics, so we can
reify the approach and only create the map of variables if there's
something evaluated.
---
 hcl2template/parser.go                | 22 +++++++++++++++++-----
 hcl2template/types.datasource_test.go | 16 ++++++++++++++++
 hcl2template/types.packer_config.go   | 17 ++++++++++-------
 hcl2template/types.variables_test.go  |  2 +-
 4 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/hcl2template/parser.go b/hcl2template/parser.go
index a6f50d153..f3c51c4f8 100644
--- a/hcl2template/parser.go
+++ b/hcl2template/parser.go
@@ -464,7 +464,19 @@ func (cfg *PackerConfig) evaluateBuildPrereqs(skipDatasources bool) hcl.Diagnost
 		case *DatasourceBlock:
 			diags = cfg.evaluateDatasource(*bl, skipDatasources)
 		case *LocalBlock:
-			diags = cfg.evaluateLocalVariable(bl)
+			var val *Variable
+			if cfg.LocalVariables == nil {
+				cfg.LocalVariables = make(Variables)
+			}
+			val, diags = cfg.evaluateLocalVariable(bl)
+			// Note: clumsy a bit, but we won't add the variable as `nil` here
+			// unless no errors have been reported during evaluation.
+			//
+			// This prevents Packer from panicking down the line, as initialisation
+			// doesn't stop if there are diags, so if `val` is nil, it crashes.
+			if !diags.HasErrors() {
+				cfg.LocalVariables[bl.LocalName] = val
+			}
 		default:
 			diags = diags.Append(&hcl.Diagnostic{
 				Severity: hcl.DiagError,
@@ -476,11 +488,11 @@ func (cfg *PackerConfig) evaluateBuildPrereqs(skipDatasources bool) hcl.Diagnost
 			})
 		}
 
-		return nil
-	}
+		if diags.HasErrors() {
+			return diags
+		}
 
-	if cfg.LocalVariables == nil {
-		cfg.LocalVariables = Variables{}
+		return nil
 	}
 
 	for _, vtx := range graph.ReverseTopologicalOrder() {
diff --git a/hcl2template/types.datasource_test.go b/hcl2template/types.datasource_test.go
index 92df218d9..f539af994 100644
--- a/hcl2template/types.datasource_test.go
+++ b/hcl2template/types.datasource_test.go
@@ -114,6 +114,10 @@ func TestParse_datasource(t *testing.T) {
 					}: {
 						Type:   "null",
 						DSName: "baz",
+						Dependencies: []refString{
+							{"data", "null", "foo"},
+							{"data", "null", "bar"},
+						},
 					},
 					{
 						Type: "null",
@@ -121,6 +125,9 @@ func TestParse_datasource(t *testing.T) {
 					}: {
 						Type:   "null",
 						DSName: "bang",
+						Dependencies: []refString{
+							{"data", "null", "baz"},
+						},
 					},
 					{
 						Type: "null",
@@ -128,6 +135,9 @@ func TestParse_datasource(t *testing.T) {
 					}: {
 						Type:   "null",
 						DSName: "yummy",
+						Dependencies: []refString{
+							{"data", "null", "bang"},
+						},
 					},
 				},
 			},
@@ -219,6 +229,9 @@ func TestParse_datasource(t *testing.T) {
 					}: {
 						Type:   "null",
 						DSName: "gummy",
+						Dependencies: []refString{
+							{"data", "null", "bear"},
+						},
 					},
 					{
 						Type: "null",
@@ -226,6 +239,9 @@ func TestParse_datasource(t *testing.T) {
 					}: {
 						Type:   "null",
 						DSName: "bear",
+						Dependencies: []refString{
+							{"data", "null", "gummy"},
+						},
 					},
 				},
 			},
diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go
index fadffe295..626ebdb7b 100644
--- a/hcl2template/types.packer_config.go
+++ b/hcl2template/types.packer_config.go
@@ -341,10 +341,15 @@ func (c *PackerConfig) recursivelyEvaluateLocalVariable(local *LocalBlock, depth
 		diags = diags.Extend(localDiags)
 	}
 
-	return diags.Extend(c.evaluateLocalVariable(local))
+	val, locDiags := c.evaluateLocalVariable(local)
+	if !locDiags.HasErrors() {
+		c.LocalVariables[local.LocalName] = val
+	}
+
+	return diags.Extend(locDiags)
 }
 
-func (cfg *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics {
+func (cfg *PackerConfig) evaluateLocalVariable(local *LocalBlock) (*Variable, hcl.Diagnostics) {
 	var diags hcl.Diagnostics
 
 	value, moreDiags := local.Expr.Value(cfg.EvalContext(LocalContext, nil))
@@ -353,9 +358,9 @@ func (cfg *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostic
 
 	diags = append(diags, moreDiags...)
 	if moreDiags.HasErrors() {
-		return diags
+		return nil, diags
 	}
-	cfg.LocalVariables[local.LocalName] = &Variable{
+	return &Variable{
 		Name:      local.LocalName,
 		Sensitive: local.Sensitive,
 		Values: []VariableAssignment{{
@@ -364,9 +369,7 @@ func (cfg *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostic
 			From:  "default",
 		}},
 		Type: value.Type(),
-	}
-
-	return diags
+	}, diags
 }
 
 func (cfg *PackerConfig) evaluateDatasources(skipExecution bool) hcl.Diagnostics {
diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go
index ecff3677c..218469fb6 100644
--- a/hcl2template/types.variables_test.go
+++ b/hcl2template/types.variables_test.go
@@ -396,7 +396,7 @@ func TestParse_variables(t *testing.T) {
 			&PackerConfig{
 				CorePackerVersionString: lockedVersion,
 				Basedir:                 filepath.Join("testdata", "variables"),
-				LocalVariables:          Variables{},
+				LocalVariables:          nil,
 			},
 			true, true,
 			[]packersdk.Build{},