From b33c353233aae4d73b1567aee41fd5d4cad5bf76 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sat, 9 Jun 2018 16:23:09 +0200 Subject: [PATCH] refactor(context): Implement more explicit merging of variables The hierarchy for loading variables was previously not expressed explicitly. This commit refactors the logic for merging variables to explicitly set the different layers of variables as values on the context object and merge them for each resource set in `mergeContextValues`. --- context/context.go | 86 ++++++++++++++-------- context/context_test.go | 60 +++++++++------ context/testdata/import-vars-override.yaml | 3 +- main.go | 7 +- templater/templater.go | 2 - 5 files changed, 96 insertions(+), 62 deletions(-) diff --git a/context/context.go b/context/context.go index 1a2e5c88c..42ad2e8c1 100644 --- a/context/context.go +++ b/context/context.go @@ -43,11 +43,17 @@ type Context struct { Global map[string]interface{} `json:"global"` // File names of YAML or JSON files including extra variables that should be globally accessible - VariableImports []string `json:"import"` + VariableImportFiles []string `json:"import"` // The resource sets to include in this context ResourceSets []ResourceSet `json:"include"` + // Variables imported from additional files + ImportedVars map[string]interface{} + + // Explicitly set variables (via `--var`) that should override all others + ExplicitVars map[string]interface{} + // This field represents the absolute path to the context base directory and should not be manually specified. BaseDir string } @@ -57,41 +63,59 @@ func contextLoadingError(filename string, cause error) error { } // Attempt to load and deserialise a Context from the specified file. -func LoadContextFromFile(filename string) (*Context, error) { - var c Context - err := util.LoadJsonOrYaml(filename, &c) +func LoadContext(filename string, explicitVars *[]string) (*Context, error) { + var ctx Context + err := util.LoadJsonOrYaml(filename, &ctx) if err != nil { return nil, contextLoadingError(filename, err) } - c.ResourceSets = flattenPrepareResourceSetPaths(&c.ResourceSets) - c.BaseDir = path.Dir(filename) - c.ResourceSets = loadAllDefaultValues(&c) + ctx.BaseDir = path.Dir(filename) - err = c.loadImportedVariables() + // Prepare the resource sets by resolving parents etc. + ctx.ResourceSets = flattenPrepareResourceSetPaths(&ctx.ResourceSets) + + // Add variables explicitly specified on the command line + ctx.ExplicitVars, err = loadExplicitVars(explicitVars) + if err != nil { + return nil, fmt.Errorf("Error setting explicit variables: %v\n", err) + } + + // Add variables loaded from import files + ctx.ImportedVars, err = ctx.loadImportedVariables() if err != nil { return nil, contextLoadingError(filename, err) } - return &c, nil + // Merge variables (explicit > import > include > global > default) + ctx.ResourceSets = ctx.mergeContextValues() + + if err != nil { + return nil, contextLoadingError(filename, err) + } + + return &ctx, nil } -// Kontemplate supports specifying additional variable files with the `import` keyword. This function loads those -// variable files and merges them together with the context's other global variables. -func (ctx *Context) loadImportedVariables() error { - for _, file := range ctx.VariableImports { +// Kontemplate supports specifying additional variable files with the +// `import` keyword. This function loads those variable files and +// merges them together with the context's other global variables. +func (ctx *Context) loadImportedVariables() (map[string]interface{}, error) { + allImportedVars := make(map[string]interface{}) + + for _, file := range ctx.VariableImportFiles { var importedVars map[string]interface{} err := util.LoadJsonOrYaml(path.Join(ctx.BaseDir, file), &importedVars) if err != nil { - return err + return nil, err } - ctx.Global = *util.Merge(&ctx.Global, &importedVars) + allImportedVars = *util.Merge(&allImportedVars, &importedVars) } - return nil + return allImportedVars, nil } // Correctly prepares the file paths for resource sets by inferring implicit paths and flattening resource set @@ -128,11 +152,16 @@ func flattenPrepareResourceSetPaths(rs *[]ResourceSet) []ResourceSet { return flattened } -func loadAllDefaultValues(c *Context) []ResourceSet { - updated := make([]ResourceSet, len(c.ResourceSets)) +// Merges the context and resource set variables according in the +// desired precedence order. +func (ctx *Context) mergeContextValues() []ResourceSet { + updated := make([]ResourceSet, len(ctx.ResourceSets)) - for i, rs := range c.ResourceSets { - merged := loadDefaultValues(&rs, c) + for i, rs := range ctx.ResourceSets { + merged := loadDefaultValues(&rs, ctx) + merged = util.Merge(&ctx.Global, merged) + merged = util.Merge(merged, &ctx.ImportedVars) + merged = util.Merge(merged, &ctx.ExplicitVars) rs.Values = *merged updated[i] = rs } @@ -160,22 +189,19 @@ func loadDefaultValues(rs *ResourceSet, c *Context) *map[string]interface{} { return &rs.Values } -// New variables can be defined or default values overridden with command line arguments when executing kontemplate. -func (ctx *Context) SetVariablesFromArguments(vars *[]string) error { - // Resource set files might not have defined any global variables, if so we have to - // create that a map before potentially writing variables into it - if ctx.Global == nil { - ctx.Global = make(map[string]interface{}, len(*vars)) - } +// Prepares the variables specified explicitly via `--var` when +// executing kontemplate for adding to the context. +func loadExplicitVars(vars *[]string) (map[string]interface{}, error) { + explicitVars := make(map[string]interface{}, len(*vars)) for _, v := range *vars { varParts := strings.Split(v, "=") if len(varParts) != 2 { - return fmt.Errorf(`invalid explicit variable provided (%s), name and value should be divided with "="`, v) + return nil, fmt.Errorf(`invalid explicit variable provided (%s), name and value should be separated with "="`, v) } - ctx.Global[varParts[0]] = varParts[1] + explicitVars[varParts[0]] = varParts[1] } - return nil + return explicitVars, nil } diff --git a/context/context_test.go b/context/context_test.go index 6dc27466a..6a4cec6c9 100644 --- a/context/context_test.go +++ b/context/context_test.go @@ -14,8 +14,10 @@ import ( "testing" ) +var noExplicitVars []string = make([]string, 0) + func TestLoadFlatContextFromFile(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/flat-test.yaml") + ctx, err := LoadContext("testdata/flat-test.yaml", &noExplicitVars) if err != nil { t.Error(err) @@ -35,12 +37,15 @@ func TestLoadFlatContextFromFile(t *testing.T) { "apiPort": float64(4567), // yep! "importantFeature": true, "version": "1.0-0e6884d", + "globalVar": "lizards", }, Include: nil, Parent: "", }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -50,7 +55,7 @@ func TestLoadFlatContextFromFile(t *testing.T) { } func TestLoadContextWithResourceSetCollections(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/collections-test.yaml") + ctx, err := LoadContext("testdata/collections-test.yaml", &noExplicitVars) if err != nil { t.Error(err) @@ -70,6 +75,7 @@ func TestLoadContextWithResourceSetCollections(t *testing.T) { "apiPort": float64(4567), // yep! "importantFeature": true, "version": "1.0-0e6884d", + "globalVar": "lizards", }, Include: nil, Parent: "", @@ -79,12 +85,15 @@ func TestLoadContextWithResourceSetCollections(t *testing.T) { Path: "collection/nested", Values: map[string]interface{}{ "lizards": "good", + "globalVar": "lizards", }, Include: nil, Parent: "collection", }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -95,7 +104,7 @@ func TestLoadContextWithResourceSetCollections(t *testing.T) { } func TestSubresourceVariableInheritance(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/parent-variables.yaml") + ctx, err := LoadContext("testdata/parent-variables.yaml", &noExplicitVars) if err != nil { t.Error(err) @@ -117,6 +126,8 @@ func TestSubresourceVariableInheritance(t *testing.T) { }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -126,7 +137,7 @@ func TestSubresourceVariableInheritance(t *testing.T) { } func TestSubresourceVariableInheritanceOverride(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/parent-variable-override.yaml") + ctx, err := LoadContext("testdata/parent-variable-override.yaml", &noExplicitVars) if err != nil { t.Error(err) @@ -147,6 +158,8 @@ func TestSubresourceVariableInheritanceOverride(t *testing.T) { }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -156,7 +169,7 @@ func TestSubresourceVariableInheritanceOverride(t *testing.T) { } func TestDefaultValuesLoading(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/default-loading.yaml") + ctx, err := LoadContext("testdata/default-loading.yaml", &noExplicitVars) if err != nil { t.Error(err) t.Fail() @@ -175,7 +188,7 @@ func TestDefaultValuesLoading(t *testing.T) { } func TestImportValuesLoading(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/import-vars-simple.yaml") + ctx, err := LoadContext("testdata/import-vars-simple.yaml", &noExplicitVars) if err != nil { t.Error(err) t.Fail() @@ -189,14 +202,14 @@ func TestImportValuesLoading(t *testing.T) { }, } - if !reflect.DeepEqual(ctx.Global, expected) { - t.Error("Expected global values after loading imports did not match!") + if !reflect.DeepEqual(ctx.ImportedVars, expected) { + t.Error("Expected imported values after loading imports did not match!") t.Fail() } } -func TestImportValuesOverride(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/import-vars-override.yaml") +func TestValuesOverride(t *testing.T) { + ctx, err := LoadContext("testdata/import-vars-override.yaml", &noExplicitVars) if err != nil { t.Error(err) t.Fail() @@ -208,18 +221,18 @@ func TestImportValuesOverride(t *testing.T) { "artist": "Pallida", "track": "Tractor Beam", }, - "place": "Oslo", + "place": "Oslo", "globalVar": "very global!", } - if !reflect.DeepEqual(ctx.Global, expected) { - t.Error("Expected global values after loading imports did not match!") + if !reflect.DeepEqual(ctx.ResourceSets[0].Values, expected) { + t.Error("Expected overrides after loading imports did not match!") t.Fail() } } func TestExplicitPathLoading(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/explicit-path.yaml") + ctx, err := LoadContext("testdata/explicit-path.yaml", &noExplicitVars) if err != nil { t.Error(err) t.Fail() @@ -248,6 +261,8 @@ func TestExplicitPathLoading(t *testing.T) { }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -257,7 +272,7 @@ func TestExplicitPathLoading(t *testing.T) { } func TestExplicitSubresourcePathLoading(t *testing.T) { - ctx, err := LoadContextFromFile("testdata/explicit-subresource-path.yaml") + ctx, err := LoadContext("testdata/explicit-subresource-path.yaml", &noExplicitVars) if err != nil { t.Error(err) t.Fail() @@ -270,9 +285,12 @@ func TestExplicitSubresourcePathLoading(t *testing.T) { Name: "parent/child", Path: "parent-path/child-path", Parent: "parent", + Values: make(map[string]interface{}, 0), }, }, BaseDir: "testdata", + ImportedVars: make(map[string]interface{}, 0), + ExplicitVars: make(map[string]interface{}, 0), } if !reflect.DeepEqual(*ctx, expected) { @@ -283,22 +301,18 @@ func TestExplicitSubresourcePathLoading(t *testing.T) { func TestSetVariablesFromArguments(t *testing.T) { vars := []string{"version=some-service-version"} - ctx, _ := LoadContextFromFile("testdata/default-loading.yaml") + ctx, _ := LoadContext("testdata/default-loading.yaml", &vars) - if err := ctx.SetVariablesFromArguments(&vars); err != nil { - t.Error(err) - } - - if version := ctx.Global["version"]; version != "some-service-version" { + if version := ctx.ExplicitVars["version"]; version != "some-service-version" { t.Errorf(`Expected variable "version" to have value "some-service-version" but was "%s"`, version) } } func TestSetInvalidVariablesFromArguments(t *testing.T) { vars := []string{"version: some-service-version"} - ctx, _ := LoadContextFromFile("testdata/default-loading.yaml") + _, err := LoadContext("testdata/default-loading.yaml", &vars) - if err := ctx.SetVariablesFromArguments(&vars); err == nil { + if err == nil { t.Error("Expected invalid variable to return an error") } } diff --git a/context/testdata/import-vars-override.yaml b/context/testdata/import-vars-override.yaml index c3d47bcfc..c749a2a66 100644 --- a/context/testdata/import-vars-override.yaml +++ b/context/testdata/import-vars-override.yaml @@ -6,4 +6,5 @@ global: import: - test-vars.yaml - test-vars-override.yaml -include: [] +include: + - name: test-set diff --git a/main.go b/main.go index ee70bdae5..b792a1a3b 100644 --- a/main.go +++ b/main.go @@ -184,16 +184,11 @@ func createCommand() { } func loadContextAndResources(file *string) (*context.Context, *[]templater.RenderedResourceSet) { - ctx, err := context.LoadContextFromFile(*file) + ctx, err := context.LoadContext(*file, variables) if err != nil { app.Fatalf("Error loading context: %v\n", err) } - err = ctx.SetVariablesFromArguments(variables) - if err != nil { - app.Fatalf("Error setting explicit variables in context: %v\n", err) - } - resources, err := templater.LoadAndApplyTemplates(includes, excludes, ctx) if err != nil { app.Fatalf("Error templating resource sets: %v\n", err) diff --git a/templater/templater.go b/templater/templater.go index c0eff234f..13ec9643e 100644 --- a/templater/templater.go +++ b/templater/templater.go @@ -111,8 +111,6 @@ func templateFile(c *context.Context, rs *context.ResourceSet, filename string) var b bytes.Buffer - rs.Values = *util.Merge(&c.Global, &rs.Values) - err = tpl.Execute(&b, rs.Values) if err != nil {