From 5110a37bae772f9b7155dd7761dedf702f065b47 Mon Sep 17 00:00:00 2001 From: Kamil Sitnik Date: Fri, 20 Jun 2025 13:59:00 -0400 Subject: [PATCH] Implement deep merge functionality for multiple variable files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when multiple variable files were provided, nested objects would be completely overwritten by subsequent files instead of being merged. This change implements proper deep merging of nested structures. Changes: - Add deepMerge() function for recursive map merging - Add convertYAMLMap() functions to handle YAML parsing edge cases - Replace simple key assignment with deep merge in variable processing - Add comprehensive tests for YAML, JSON, and Terraform variable files - Test cases verify proper merging of nested database, cache, and logging configs Fixes variable file overwriting bug where providing two variable files resulted in loss of nested configuration from the first file. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- template/render.go | 68 +++++- template/render_test.go | 202 ++++++++++++++++++ template/test-fixtures/nested_templated.nomad | 23 ++ template/test-fixtures/test-nested-1.json | 13 ++ template/test-fixtures/test-nested-1.tf | 16 ++ template/test-fixtures/test-nested-1.yaml | 11 + template/test-fixtures/test-nested-2.json | 11 + template/test-fixtures/test-nested-2.tf | 11 + template/test-fixtures/test-nested-2.yaml | 9 + 9 files changed, 361 insertions(+), 3 deletions(-) create mode 100644 template/test-fixtures/nested_templated.nomad create mode 100644 template/test-fixtures/test-nested-1.json create mode 100644 template/test-fixtures/test-nested-1.tf create mode 100644 template/test-fixtures/test-nested-1.yaml create mode 100644 template/test-fixtures/test-nested-2.json create mode 100644 template/test-fixtures/test-nested-2.tf create mode 100644 template/test-fixtures/test-nested-2.yaml diff --git a/template/render.go b/template/render.go index 9533c0e69..184f1c9cc 100644 --- a/template/render.go +++ b/template/render.go @@ -80,9 +80,7 @@ func RenderTemplate(templateFile string, variableFiles []string, addr string, fl if err != nil { return } - for k, v := range variables { - mergedVariables[k] = v - } + deepMerge(mergedVariables, variables) } src, err := ioutil.ReadFile(t.jobTemplateFile) @@ -145,6 +143,9 @@ func (t *tmpl) parseYAMLVars(variableFile string) (variables map[string]interfac if err = yaml.Unmarshal(yamlFile, &variables); err != nil { return } + + // Convert any map[interface{}]interface{} to map[string]interface{} for proper deep merge + variables = convertYAMLMap(variables) return variables, nil } @@ -169,3 +170,64 @@ func (t *tmpl) renderTemplate(src string, variables map[string]interface{}) (tpl return tpl, err } + +// convertYAMLMap recursively converts map[interface{}]interface{} to map[string]interface{} +// This is needed because YAML unmarshaling can create mixed key types +func convertYAMLMap(input map[string]interface{}) map[string]interface{} { + result := make(map[string]interface{}) + for key, value := range input { + result[key] = convertYAMLValue(value) + } + return result +} + +// convertYAMLValue converts interface{} values, handling nested maps and slices +func convertYAMLValue(value interface{}) interface{} { + switch v := value.(type) { + case map[interface{}]interface{}: + // Convert map[interface{}]interface{} to map[string]interface{} + result := make(map[string]interface{}) + for k, val := range v { + if keyStr, ok := k.(string); ok { + result[keyStr] = convertYAMLValue(val) + } + } + return result + case map[string]interface{}: + // Already the right type, but recursively convert values + result := make(map[string]interface{}) + for k, val := range v { + result[k] = convertYAMLValue(val) + } + return result + case []interface{}: + // Convert slice elements + result := make([]interface{}, len(v)) + for i, val := range v { + result[i] = convertYAMLValue(val) + } + return result + default: + // Return as-is for primitive types + return value + } +} + +// deepMerge recursively merges src map into dst map, handling nested structures +func deepMerge(dst, src map[string]interface{}) { + for key, srcVal := range src { + if dstVal, exists := dst[key]; exists { + // Both values exist, check if they are both maps + if dstMap, dstIsMap := dstVal.(map[string]interface{}); dstIsMap { + if srcMap, srcIsMap := srcVal.(map[string]interface{}); srcIsMap { + // Both are maps, merge recursively + deepMerge(dstMap, srcMap) + continue + } + } + } + // Either key doesn't exist in dst, or one of the values is not a map + // In either case, overwrite with src value + dst[key] = srcVal + } +} diff --git a/template/render_test.go b/template/render_test.go index 3c4b0e1ce..b5a2ccf22 100644 --- a/template/render_test.go +++ b/template/render_test.go @@ -119,3 +119,205 @@ func TestTemplater_RenderTemplate(t *testing.T) { t.Fatalf("expected %s but got %v", testEnvValue, *job.TaskGroups[0].Name) } } + +func TestTemplater_DeepMergeVariables(t *testing.T) { + // Test deep merge functionality with nested variables + fVars := make(map[string]interface{}) + + job, err := RenderJob("test-fixtures/nested_templated.nomad", []string{"test-fixtures/test-nested-1.yaml", "test-fixtures/test-nested-2.yaml"}, "", &fVars) + if err != nil { + t.Fatal(err) + } + + if *job.Name != testJobName { + t.Fatalf("expected %s but got %v", testJobName, *job.Name) + } + + // Check that the job config was properly rendered (this validates the deep merge worked) + config := job.TaskGroups[0].Tasks[0].Config + args, ok := config["args"].([]interface{}) + if !ok { + t.Fatal("expected args to be a slice") + } + + // Check that we have the expected number of args + if len(args) != 7 { + t.Fatalf("expected 7 args but got %d", len(args)) + } + + // Check that database config from both files is present + found := make(map[string]bool) + expected := []string{"DB Host: localhost", "DB Port: 5432", "DB User: admin", "DB Pass: secret", "Cache Enabled: true", "Cache TTL: 300", "Log Level: debug"} + + for _, arg := range args { + argStr := arg.(string) + for _, exp := range expected { + if argStr == exp { + found[exp] = true + } + } + } + + for _, exp := range expected { + if !found[exp] { + t.Errorf("expected argument '%s' not found in job config. Got args: %v", exp, args) + } + } +} + +func TestDeepMerge(t *testing.T) { + // Test the deepMerge function directly + dst := map[string]interface{}{ + "config": map[string]interface{}{ + "database": map[string]interface{}{ + "host": "localhost", + "port": 5432, + }, + "cache": map[string]interface{}{ + "enabled": true, + "ttl": 300, + }, + }, + "job_name": "levantExample", + } + + src := map[string]interface{}{ + "config": map[string]interface{}{ + "database": map[string]interface{}{ + "username": "admin", + "password": "secret", + }, + "logging": map[string]interface{}{ + "level": "debug", + }, + }, + } + + deepMerge(dst, src) + + // Check that the merge worked correctly + config, ok := dst["config"].(map[string]interface{}) + if !ok { + t.Fatal("config should be a map") + } + + database, ok := config["database"].(map[string]interface{}) + if !ok { + t.Fatal("database should be a map") + } + + // Check all database fields are present + if database["host"] != "localhost" { + t.Errorf("expected host localhost, got %v", database["host"]) + } + if database["port"] != 5432 { + t.Errorf("expected port 5432, got %v", database["port"]) + } + if database["username"] != "admin" { + t.Errorf("expected username admin, got %v", database["username"]) + } + if database["password"] != "secret" { + t.Errorf("expected password secret, got %v", database["password"]) + } + + // Check cache section is preserved + cache, ok := config["cache"].(map[string]interface{}) + if !ok { + t.Fatal("cache should be a map") + } + if cache["enabled"] != true { + t.Errorf("expected cache enabled true, got %v", cache["enabled"]) + } + if cache["ttl"] != 300 { + t.Errorf("expected cache ttl 300, got %v", cache["ttl"]) + } + + // Check logging section is added + logging, ok := config["logging"].(map[string]interface{}) + if !ok { + t.Fatal("logging should be a map") + } + if logging["level"] != "debug" { + t.Errorf("expected logging level debug, got %v", logging["level"]) + } +} + +func TestTemplater_DeepMergeJSON(t *testing.T) { + // Test deep merge functionality with JSON files + fVars := make(map[string]interface{}) + + job, err := RenderJob("test-fixtures/nested_templated.nomad", []string{"test-fixtures/test-nested-1.json", "test-fixtures/test-nested-2.json"}, "", &fVars) + if err != nil { + t.Fatal(err) + } + + if *job.Name != testJobName { + t.Fatalf("expected %s but got %v", testJobName, *job.Name) + } + + // Verify the nested variables were properly merged for JSON + config := job.TaskGroups[0].Tasks[0].Config + args, ok := config["args"].([]interface{}) + if !ok { + t.Fatal("expected args to be a slice") + } + + // Check that database config from both files is present + found := make(map[string]bool) + expected := []string{"DB Host: localhost", "DB Port: 5432", "DB User: admin", "DB Pass: secret", "Cache Enabled: true", "Cache TTL: 300", "Log Level: debug"} + + for _, arg := range args { + argStr := arg.(string) + for _, exp := range expected { + if argStr == exp { + found[exp] = true + } + } + } + + for _, exp := range expected { + if !found[exp] { + t.Errorf("expected argument '%s' not found in job config. Got args: %v", exp, args) + } + } +} + +func TestTemplater_DeepMergeTerraform(t *testing.T) { + // Test deep merge functionality with Terraform files + fVars := make(map[string]interface{}) + + job, err := RenderJob("test-fixtures/nested_templated.nomad", []string{"test-fixtures/test-nested-1.tf", "test-fixtures/test-nested-2.tf"}, "", &fVars) + if err != nil { + t.Fatal(err) + } + + if *job.Name != testJobName { + t.Fatalf("expected %s but got %v", testJobName, *job.Name) + } + + // Verify the nested variables were properly merged for Terraform + config := job.TaskGroups[0].Tasks[0].Config + args, ok := config["args"].([]interface{}) + if !ok { + t.Fatal("expected args to be a slice") + } + + // Check that database config from both files is present + found := make(map[string]bool) + expected := []string{"DB Host: localhost", "DB Port: 5432", "DB User: admin", "DB Pass: secret", "Cache Enabled: true", "Cache TTL: 300", "Log Level: debug"} + + for _, arg := range args { + argStr := arg.(string) + for _, exp := range expected { + if argStr == exp { + found[exp] = true + } + } + } + + for _, exp := range expected { + if !found[exp] { + t.Errorf("expected argument '%s' not found in job config. Got args: %v", exp, args) + } + } +} diff --git a/template/test-fixtures/nested_templated.nomad b/template/test-fixtures/nested_templated.nomad new file mode 100644 index 000000000..b52b11c8e --- /dev/null +++ b/template/test-fixtures/nested_templated.nomad @@ -0,0 +1,23 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +job "[[.job_name]]" { + datacenters = ["dc1"] + + task "test" { + driver = "exec" + + config { + command = "echo" + args = [ + "DB Host: [[.config.database.host]]", + "DB Port: [[.config.database.port]]", + "DB User: [[.config.database.username]]", + "DB Pass: [[.config.database.password]]", + "Cache Enabled: [[.config.cache.enabled]]", + "Cache TTL: [[.config.cache.ttl]]", + "Log Level: [[.config.logging.level]]" + ] + } + } +} \ No newline at end of file diff --git a/template/test-fixtures/test-nested-1.json b/template/test-fixtures/test-nested-1.json new file mode 100644 index 000000000..d6dbeacaa --- /dev/null +++ b/template/test-fixtures/test-nested-1.json @@ -0,0 +1,13 @@ +{ + "job_name": "levantExample", + "config": { + "database": { + "host": "localhost", + "port": 5432 + }, + "cache": { + "enabled": true, + "ttl": 300 + } + } +} \ No newline at end of file diff --git a/template/test-fixtures/test-nested-1.tf b/template/test-fixtures/test-nested-1.tf new file mode 100644 index 000000000..a573fa633 --- /dev/null +++ b/template/test-fixtures/test-nested-1.tf @@ -0,0 +1,16 @@ +variable "job_name" { + default = "levantExample" +} + +variable "config" { + default = { + database = { + host = "localhost" + port = 5432 + } + cache = { + enabled = true + ttl = 300 + } + } +} \ No newline at end of file diff --git a/template/test-fixtures/test-nested-1.yaml b/template/test-fixtures/test-nested-1.yaml new file mode 100644 index 000000000..b13c2ecaa --- /dev/null +++ b/template/test-fixtures/test-nested-1.yaml @@ -0,0 +1,11 @@ +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: MPL-2.0 + +job_name: levantExample +config: + database: + host: localhost + port: 5432 + cache: + enabled: true + ttl: 300 \ No newline at end of file diff --git a/template/test-fixtures/test-nested-2.json b/template/test-fixtures/test-nested-2.json new file mode 100644 index 000000000..691cb10eb --- /dev/null +++ b/template/test-fixtures/test-nested-2.json @@ -0,0 +1,11 @@ +{ + "config": { + "database": { + "username": "admin", + "password": "secret" + }, + "logging": { + "level": "debug" + } + } +} \ No newline at end of file diff --git a/template/test-fixtures/test-nested-2.tf b/template/test-fixtures/test-nested-2.tf new file mode 100644 index 000000000..756a00d6a --- /dev/null +++ b/template/test-fixtures/test-nested-2.tf @@ -0,0 +1,11 @@ +variable "config" { + default = { + database = { + username = "admin" + password = "secret" + } + logging = { + level = "debug" + } + } +} \ No newline at end of file diff --git a/template/test-fixtures/test-nested-2.yaml b/template/test-fixtures/test-nested-2.yaml new file mode 100644 index 000000000..c7f6f9dfc --- /dev/null +++ b/template/test-fixtures/test-nested-2.yaml @@ -0,0 +1,9 @@ +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: MPL-2.0 + +config: + database: + username: admin + password: secret + logging: + level: debug \ No newline at end of file