From 292d07d13c4cbf47fac7ed3e3b268699d60112ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Fri, 25 Mar 2022 16:40:58 +0100 Subject: [PATCH] Manager: erase configured variables when overlapping with implicit vars When a variable is found in the config file, with the same name as an implicit variable, it will be removed from the configuration (i.e. implicit ones always win). A warning is logged when this happens. --- internal/manager/config/settings.go | 48 +++++++++++++++++++----- internal/manager/config/settings_test.go | 19 ++++++++-- 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/internal/manager/config/settings.go b/internal/manager/config/settings.go index 43f0c4f2..d3b24386 100644 --- a/internal/manager/config/settings.go +++ b/internal/manager/config/settings.go @@ -131,6 +131,10 @@ type Conf struct { // Variable name → Variable definition Variables map[string]Variable `yaml:"variables"` + // Implicit variables work as regular variables, but do not get written to the + // configuration file. + implicitVariables map[string]Variable `yaml:"-"` + // audience + platform + variable name → variable value. // Used to look up variables for a given platform and audience. // The 'audience' is never "all" or ""; only concrete audiences are stored here. @@ -182,6 +186,7 @@ func DefaultConfig(override ...func(c *Conf)) Conf { overrideFunc(&c) } c.addImplicitVariables() + c.ensureVariablesUnique() c.constructVariableLookupTable(zerolog.TraceLevel) return c } @@ -225,6 +230,7 @@ func loadConf(filename string, overrides ...func(c *Conf)) (Conf, error) { } c.addImplicitVariables() + c.ensureVariablesUnique() c.constructVariableLookupTable(zerolog.DebugLevel) c.parseURLs() c.checkDatabase() @@ -235,6 +241,8 @@ func loadConf(filename string, overrides ...func(c *Conf)) (Conf, error) { } func (c *Conf) addImplicitVariables() { + c.implicitVariables = make(map[string]Variable) + if !c.Shaman.Enabled { return } @@ -247,7 +255,7 @@ func (c *Conf) addImplicitVariables() { log.Error().Err(err).Msg("unable to find absolute path of Shaman checkout path") absPath = shamanCheckoutPath } - c.Variables["jobs"] = Variable{ + c.implicitVariables["jobs"] = Variable{ IsTwoWay: false, Values: []VariableValue{ { @@ -259,9 +267,33 @@ func (c *Conf) addImplicitVariables() { } } -func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { - lookup := map[VariableAudience]map[VariablePlatform]map[string]string{} +// ensureVariablesUnique erases configured variables when there are implicit +// variables with the same name. +func (c *Conf) ensureVariablesUnique() { + for varname := range c.implicitVariables { + if _, found := c.Variables[varname]; !found { + continue + } + log.Warn().Str("variable", varname). + Msg("configured variable will be removed, as there is an implicit variable with the same name") + delete(c.Variables, varname) + } +} +func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { + if c.VariablesLookup == nil { + c.VariablesLookup = map[VariableAudience]map[VariablePlatform]map[string]string{} + } + + c.constructVariableLookupTableForVars(logLevel, c.Variables) + c.constructVariableLookupTableForVars(logLevel, c.implicitVariables) + + log.Trace(). + Interface("variables", c.Variables). + Msg("constructed lookup table") +} + +func (c *Conf) constructVariableLookupTableForVars(logLevel zerolog.Level, vars map[string]Variable) { // Construct a list of all audiences except "" and "all" concreteAudiences := []VariableAudience{} isWildcard := map[VariableAudience]bool{"": true, VariableAudienceAll: true} @@ -276,6 +308,9 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { Interface("isWildcard", isWildcard). Msg("constructing variable lookup table") + // Just for brevity. + lookup := c.VariablesLookup + // setValue expands wildcard audiences into concrete ones. var setValue func(audience VariableAudience, platform VariablePlatform, name, value string) setValue = func(audience VariableAudience, platform VariablePlatform, name, value string) { @@ -302,7 +337,7 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { } // Construct the lookup table for each audience+platform+name - for name, variable := range c.Variables { + for name, variable := range vars { log.WithLevel(logLevel). Str("name", name). Interface("variable", variable). @@ -332,11 +367,6 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { } } } - log.Trace(). - Interface("variables", c.Variables). - Interface("lookup", lookup). - Msg("constructed lookup table") - c.VariablesLookup = lookup } func updateMap[K comparable, V any](target map[K]V, updateWith map[K]V) { diff --git a/internal/manager/config/settings_test.go b/internal/manager/config/settings_test.go index 6220dd71..1866e36d 100644 --- a/internal/manager/config/settings_test.go +++ b/internal/manager/config/settings_test.go @@ -46,11 +46,24 @@ func TestShamanImplicitVariables(t *testing.T) { // Having the Shaman enabled should create an implicit variable "{jobs}". c.Shaman.Enabled = true c.Shaman.StoragePath = "/path/to/shaman/storage" + + c.Variables["jobs"] = Variable{ + IsTwoWay: true, + Values: []VariableValue{ + { + Audience: VariableAudienceAll, + Platform: VariablePlatformAll, + Value: "this value should not be seen", + }, + }, + } + }) - if !assert.Contains(t, c.Variables, "jobs") { + assert.NotContains(t, c.Variables, "jobs", "implicit variables should erase existing variables with the same name") + if !assert.Contains(t, c.implicitVariables, "jobs") { t.FailNow() } - assert.False(t, c.Variables["jobs"].IsTwoWay) - assert.Equal(t, c.Shaman.CheckoutPath(), c.Variables["jobs"].Values[0].Value) + assert.False(t, c.implicitVariables["jobs"].IsTwoWay) + assert.Equal(t, c.Shaman.CheckoutPath(), c.implicitVariables["jobs"].Values[0].Value) }