From ae5846b3d920c056e10e31624da2f91eb77df9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 21 Feb 2022 18:50:24 +0100 Subject: [PATCH] Manager config: remove 'mode' and change 'variable audience' to custom type The 'variable audience' indicates the audience of a 'task variable'. --- internal/manager/api_impl/api_impl.go | 2 +- .../api_impl/mocks/api_impl_mock.gen.go | 3 +- internal/manager/api_impl/varrepl.go | 3 +- internal/manager/config/enums.go | 30 ++++++++ internal/manager/config/service.go | 2 +- internal/manager/config/settings.go | 73 +++++-------------- 6 files changed, 54 insertions(+), 59 deletions(-) create mode 100644 internal/manager/config/enums.go diff --git a/internal/manager/api_impl/api_impl.go b/internal/manager/api_impl/api_impl.go index c497f7a3..4db62bbe 100644 --- a/internal/manager/api_impl/api_impl.go +++ b/internal/manager/api_impl/api_impl.go @@ -70,7 +70,7 @@ type LogStorage interface { } type ConfigService interface { - ExpandVariables(valueToExpand, audience, platform string) string + VariableReplacer } var _ api.ServerInterface = (*Flamenco)(nil) 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 c6ab90f1..d14b01e8 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -10,6 +10,7 @@ import ( gomock "github.com/golang/mock/gomock" zerolog "github.com/rs/zerolog" + config "gitlab.com/blender/flamenco-ng-poc/internal/manager/config" job_compilers "gitlab.com/blender/flamenco-ng-poc/internal/manager/job_compilers" persistence "gitlab.com/blender/flamenco-ng-poc/internal/manager/persistence" api "gitlab.com/blender/flamenco-ng-poc/pkg/api" @@ -293,7 +294,7 @@ func (m *MockConfigService) EXPECT() *MockConfigServiceMockRecorder { } // ExpandVariables mocks base method. -func (m *MockConfigService) ExpandVariables(arg0, arg1, arg2 string) string { +func (m *MockConfigService) ExpandVariables(arg0 string, arg1 config.VariableAudience, arg2 string) 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 bc5816fc..30d124e7 100644 --- a/internal/manager/api_impl/varrepl.go +++ b/internal/manager/api_impl/varrepl.go @@ -23,13 +23,14 @@ package api_impl import ( "reflect" + "gitlab.com/blender/flamenco-ng-poc/internal/manager/config" "gitlab.com/blender/flamenco-ng-poc/internal/manager/persistence" ) var stringType = reflect.TypeOf("somestring") type VariableReplacer interface { - ExpandVariables(valueToExpand, audience, platform string) string + ExpandVariables(valueToExpand string, audience config.VariableAudience, platform string) string } // replaceTaskVariables performs variable replacement for worker tasks. diff --git a/internal/manager/config/enums.go b/internal/manager/config/enums.go new file mode 100644 index 00000000..13fe539f --- /dev/null +++ b/internal/manager/config/enums.go @@ -0,0 +1,30 @@ +package config + +/* ***** BEGIN GPL LICENSE BLOCK ***** + * + * Original Code Copyright (C) 2022 Blender Foundation. + * + * This file is part of Flamenco. + * + * Flamenco is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, either version 3 of the License, or (at your option) any later + * version. + * + * Flamenco is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along with + * Flamenco. If not, see . + * + * ***** END GPL LICENSE BLOCK ***** */ + +const ( + // The "audience" of task variables. + VariableAudienceAll VariableAudience = "all" + VariableAudienceWorkers VariableAudience = "workers" + VariableAudienceUsers VariableAudience = "users" +) + +type VariableAudience string diff --git a/internal/manager/config/service.go b/internal/manager/config/service.go index 8a4f0d9b..7b39c3eb 100644 --- a/internal/manager/config/service.go +++ b/internal/manager/config/service.go @@ -38,7 +38,7 @@ func (s *Service) Load() error { return nil } -func (s *Service) ExpandVariables(valueToExpand, audience, platform string) string { +func (s *Service) ExpandVariables(valueToExpand string, audience VariableAudience, platform string) string { return s.config.ExpandVariables(valueToExpand, audience, platform) } diff --git a/internal/manager/config/settings.go b/internal/manager/config/settings.go index 9ebd8fcf..def1c7b9 100644 --- a/internal/manager/config/settings.go +++ b/internal/manager/config/settings.go @@ -54,17 +54,11 @@ var ( // ErrBadDirection is returned when a direction doesn't match "oneway" or "twoway" ErrBadDirection = errors.New("variable's direction is invalid") - // Valid values for the "mode" config variable. - validModes = map[string]bool{ - "develop": true, - "production": true, - } - // Valid values for the "audience" tag of a ConfV2 variable. - validAudiences = map[string]bool{ - "all": true, - "workers": true, - "users": true, + validAudiences = map[VariableAudience]bool{ + VariableAudienceAll: true, + VariableAudienceWorkers: true, + VariableAudienceUsers: true, } // The default configuration, use DefaultConfig() to obtain a copy. @@ -72,7 +66,6 @@ var ( Base: Base{ Meta: ConfMeta{Version: latestConfigVersion}, - Mode: "production", ManagerName: "Flamenco Manager", Listen: ":8080", ListenHTTPS: ":8433", @@ -178,7 +171,6 @@ type ConfMeta struct { type Base struct { Meta ConfMeta `yaml:"_meta"` - Mode string `yaml:"mode"` // either "develop" or "production" ManagerName string `yaml:"manager_name"` DatabaseDSN string `yaml:"database_url"` TaskLogsPath string `yaml:"task_logs_path"` @@ -254,7 +246,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[string]map[string]map[string]string `yaml:"-"` + VariablesLookup map[VariableAudience]map[string]map[string]string `yaml:"-"` } // Variable defines a configuration variable. @@ -271,7 +263,7 @@ type VariableValues []VariableValue // VariableValue defines which audience and platform see which value. type VariableValue struct { // Audience defines who will use this variable, either "all", "workers", or "users". Empty string is "all". - Audience string `yaml:"audience,omitempty" json:"audience,omitempty"` + 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"` @@ -331,7 +323,6 @@ func loadConf(filename string) (Conf, error) { c.constructVariableLookupTable() c.parseURLs() - c.checkMode(c.Mode) c.checkDatabase() c.checkVariables() c.checkTLS() @@ -340,11 +331,11 @@ func loadConf(filename string) (Conf, error) { } func (c *Conf) constructVariableLookupTable() { - lookup := map[string]map[string]map[string]string{} + lookup := map[VariableAudience]map[string]map[string]string{} // Construct a list of all audiences except "" and "all" - concreteAudiences := []string{} - isWildcard := map[string]bool{"": true, "all": true} + concreteAudiences := []VariableAudience{} + isWildcard := map[VariableAudience]bool{"": true, VariableAudienceAll: true} for audience := range validAudiences { if isWildcard[audience] { continue @@ -352,13 +343,13 @@ func (c *Conf) constructVariableLookupTable() { concreteAudiences = append(concreteAudiences, audience) } log.Debug(). - Strs("concreteAudiences", concreteAudiences). + Interface("concreteAudiences", concreteAudiences). Interface("isWildcard", isWildcard). Msg("constructing variable lookup table") // setValue expands wildcard audiences into concrete ones. - var setValue func(audience, platform, name, value string) - setValue = func(audience, platform, name, value string) { + var setValue func(audience VariableAudience, platform, name, value string) + setValue = func(audience VariableAudience, platform, name, value string) { if isWildcard[audience] { for _, aud := range concreteAudiences { setValue(aud, platform, name, value) @@ -373,7 +364,7 @@ func (c *Conf) constructVariableLookupTable() { lookup[audience][platform] = map[string]string{} } log.Debug(). - Str("audience", audience). + Str("audience", string(audience)). Str("platform", platform). Str("name", name). Str("value", value). @@ -396,7 +387,7 @@ func (c *Conf) constructVariableLookupTable() { if strings.Contains(value.Value, "\\") { log.Warn(). Str("variable", name). - Str("audience", value.Audience). + Str("audience", string(value.Audience)). Str("platform", value.Platform). Str("value", value.Value). Msg("Backslash found in variable value. Change paths to use forward slashes instead.") @@ -420,12 +411,12 @@ func (c *Conf) constructVariableLookupTable() { } // ExpandVariables converts "{variable name}" to the value that belongs to the given audience and platform. -func (c *Conf) ExpandVariables(valueToExpand, audience, platform string) string { +func (c *Conf) ExpandVariables(valueToExpand string, audience VariableAudience, platform string) string { audienceMap := c.VariablesLookup[audience] if audienceMap == nil { log.Warn(). Str("valueToExpand", valueToExpand). - Str("audience", audience). + Str("audience", string(audience)). Str("platform", platform). Msg("no variables defined for this audience") return valueToExpand @@ -435,7 +426,7 @@ func (c *Conf) ExpandVariables(valueToExpand, audience, platform string) string if platformMap == nil { log.Warn(). Str("valueToExpand", valueToExpand). - Str("audience", audience). + Str("audience", string(audience)). Str("platform", platform). Msg("no variables defined for this platform given this audience") return valueToExpand @@ -500,7 +491,7 @@ func (c *Conf) checkVariables() error { log.Error(). Str("name", name). Interface("value", value). - Str("audience", value.Audience). + Str("audience", string(value.Audience)). Msg("variable invalid audience") } @@ -573,34 +564,6 @@ func (c *Conf) HasTLS() bool { return c.ACMEDomainName != "" || c.HasCustomTLS() } -// OverrideMode checks the mode parameter for validity and logs that it's being overridden. -func (c *Conf) OverrideMode(mode string) { - if mode == c.Mode { - log.Warn().Str("mode", mode).Msg("trying to override run mode with current value; ignoring") - return - } - c.checkMode(mode) - log.Warn(). - Str("configured_mode", c.Mode). - Str("current_mode", mode). - Msg("overriding run mode") - c.Mode = mode -} - -func (c *Conf) checkMode(mode string) { - // Check mode for validity - if !validModes[mode] { - keys := make([]string, 0, len(validModes)) - for k := range validModes { - keys = append(keys, k) - } - log.Error(). - Strs("valid_values", keys). - Str("current_value", mode). - Msg("bad value for 'mode' configuration parameter") - } -} - func (c *Conf) checkTLS() { hasTLS := c.HasCustomTLS()