From 98a5d48611427d1a6d9fa5142aa72aeb86d72ecd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Fri, 25 Mar 2022 16:12:41 +0100 Subject: [PATCH] Manager: make the 'platform' of a variable its own type This prevents too many `string` types; those are now just used for the variable name & value, whereas the platform is a `VariablePlatform` type. --- .../api_impl/mocks/api_impl_mock.gen.go | 2 +- internal/manager/api_impl/varrepl.go | 4 +-- internal/manager/config/enums.go | 10 +++++++ internal/manager/config/service.go | 2 +- internal/manager/config/settings.go | 26 +++++++++---------- internal/manager/config/settings_test.go | 2 +- 6 files changed, 28 insertions(+), 18 deletions(-) diff --git a/internal/manager/api_impl/mocks/api_impl_mock.gen.go b/internal/manager/api_impl/mocks/api_impl_mock.gen.go index 0a8f3de2..2b33d25d 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -324,7 +324,7 @@ func (m *MockConfigService) EXPECT() *MockConfigServiceMockRecorder { } // ExpandVariables mocks base method. -func (m *MockConfigService) ExpandVariables(arg0 string, arg1 config.VariableAudience, arg2 string) string { +func (m *MockConfigService) ExpandVariables(arg0 string, arg1 config.VariableAudience, arg2 config.VariablePlatform) string { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ExpandVariables", arg0, arg1, arg2) ret0, _ := ret[0].(string) diff --git a/internal/manager/api_impl/varrepl.go b/internal/manager/api_impl/varrepl.go index 42cdc903..6d62f65e 100644 --- a/internal/manager/api_impl/varrepl.go +++ b/internal/manager/api_impl/varrepl.go @@ -9,13 +9,13 @@ import ( ) type VariableReplacer interface { - ExpandVariables(valueToExpand string, audience config.VariableAudience, platform string) string + ExpandVariables(valueToExpand string, audience config.VariableAudience, platform config.VariablePlatform) string } // replaceTaskVariables performs variable replacement for worker tasks. func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, worker persistence.Worker) api.AssignedTask { repl := func(value string) string { - return replacer.ExpandVariables(value, "workers", worker.Platform) + return replacer.ExpandVariables(value, "workers", config.VariablePlatform(worker.Platform)) } for cmdIndex, cmd := range task.Commands { diff --git a/internal/manager/config/enums.go b/internal/manager/config/enums.go index 5e27debf..434bd3ba 100644 --- a/internal/manager/config/enums.go +++ b/internal/manager/config/enums.go @@ -10,3 +10,13 @@ const ( ) type VariableAudience string + +const ( + // the "platform" of task variables. It's a free-form string field, but it has + // some predefined values here. + VariablePlatformLinux VariablePlatform = "linux" + VariablePlatformWindows VariablePlatform = "windows" + VariablePlatformDarwin VariablePlatform = "darwin" +) + +type VariablePlatform string diff --git a/internal/manager/config/service.go b/internal/manager/config/service.go index 7877a4b1..df3f9c5d 100644 --- a/internal/manager/config/service.go +++ b/internal/manager/config/service.go @@ -24,7 +24,7 @@ func (s *Service) Load() error { return nil } -func (s *Service) ExpandVariables(valueToExpand string, audience VariableAudience, platform string) string { +func (s *Service) ExpandVariables(valueToExpand string, audience VariableAudience, platform VariablePlatform) string { return s.config.ExpandVariables(valueToExpand, audience, platform) } diff --git a/internal/manager/config/settings.go b/internal/manager/config/settings.go index c0441638..2e62a3bc 100644 --- a/internal/manager/config/settings.go +++ b/internal/manager/config/settings.go @@ -133,7 +133,7 @@ type Conf struct { // 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. - VariablesLookup map[VariableAudience]map[string]map[string]string `yaml:"-"` + VariablesLookup map[VariableAudience]map[VariablePlatform]map[string]string `yaml:"-"` } // Variable defines a configuration variable. @@ -152,8 +152,8 @@ type VariableValue struct { Audience VariableAudience `yaml:"audience,omitempty" json:"audience,omitempty"` // Platforms that use this value. Only one of "Platform" and "Platforms" may be set. - Platform string `yaml:"platform,omitempty" json:"platform,omitempty"` - Platforms []string `yaml:"platforms,omitempty,flow" json:"platforms,omitempty"` + Platform VariablePlatform `yaml:"platform,omitempty" json:"platform,omitempty"` + Platforms []VariablePlatform `yaml:"platforms,omitempty,flow" json:"platforms,omitempty"` // The actual value of the variable for this audience+platform. Value string `yaml:"value" json:"value"` @@ -225,7 +225,7 @@ func loadConf(filename string) (Conf, error) { } func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { - lookup := map[VariableAudience]map[string]map[string]string{} + lookup := map[VariableAudience]map[VariablePlatform]map[string]string{} // Construct a list of all audiences except "" and "all" concreteAudiences := []VariableAudience{} @@ -242,8 +242,8 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { Msg("constructing variable lookup table") // setValue expands wildcard audiences into concrete ones. - var setValue func(audience VariableAudience, platform, name, value string) - setValue = func(audience VariableAudience, platform, name, value string) { + var setValue func(audience VariableAudience, platform VariablePlatform, name, value string) + setValue = func(audience VariableAudience, platform VariablePlatform, name, value string) { if isWildcard[audience] { for _, aud := range concreteAudiences { setValue(aud, platform, name, value) @@ -252,14 +252,14 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { } if lookup[audience] == nil { - lookup[audience] = map[string]map[string]string{} + lookup[audience] = map[VariablePlatform]map[string]string{} } if lookup[audience][platform] == nil { lookup[audience][platform] = map[string]string{} } log.Trace(). Str("audience", string(audience)). - Str("platform", platform). + Str("platform", string(platform)). Str("name", name). Str("value", value). Msg("setting variable") @@ -282,7 +282,7 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { log.Warn(). Str("variable", name). Str("audience", string(value.Audience)). - Str("platform", value.Platform). + Str("platform", string(value.Platform)). Str("value", value.Value). Msg("Backslash found in variable value. Change paths to use forward slashes instead.") } @@ -305,7 +305,7 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) { } // ExpandVariables converts "{variable name}" to the value that belongs to the given audience and platform. -func (c *Conf) ExpandVariables(valueToExpand string, audience VariableAudience, platform string) string { +func (c *Conf) ExpandVariables(valueToExpand string, audience VariableAudience, platform VariablePlatform) string { audienceMap := c.VariablesLookup[audience] if audienceMap == nil { log.Warn(). @@ -321,7 +321,7 @@ func (c *Conf) ExpandVariables(valueToExpand string, audience VariableAudience, log.Warn(). Str("valueToExpand", valueToExpand). Str("audience", string(audience)). - Str("platform", platform). + Str("platform", string(platform)). Msg("no variables defined for this platform given this audience") return valueToExpand } @@ -354,8 +354,8 @@ func (c *Conf) checkVariables() { log.Warn(). Str("name", name). Interface("value", value). - Str("platform", value.Platform). - Strs("platforms", value.Platforms). + Str("platform", string(value.Platform)). + Interface("platforms", value.Platforms). Msg("variable has a both 'platform' and 'platforms' set") value.Platforms = append(value.Platforms, value.Platform) value.Platform = "" diff --git a/internal/manager/config/settings_test.go b/internal/manager/config/settings_test.go index 6073166b..b701b235 100644 --- a/internal/manager/config/settings_test.go +++ b/internal/manager/config/settings_test.go @@ -19,7 +19,7 @@ func TestDefaultSettings(t *testing.T) { assert.Equal(t, false, config.Variables["ffmpeg"].IsTwoWay) assert.Equal(t, "ffmpeg", config.Variables["ffmpeg"].Values[0].Value) - assert.Equal(t, "linux", config.Variables["ffmpeg"].Values[0].Platform) + assert.Equal(t, VariablePlatformLinux, config.Variables["ffmpeg"].Values[0].Platform) } func TestVariableValidation(t *testing.T) {