From 7dc3def1d5eeee70e8848d53a21ce19cb1f04147 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 31 Jul 2023 14:47:47 +0200 Subject: [PATCH] Manager: simplify variable expansion Simplify the variable expansion code. Instead of using a separate goroutine and two channels, use a struct + a simple function call. No functional changes. --- internal/manager/api_impl/meta.go | 10 +-- internal/manager/api_impl/meta_test.go | 24 +++---- .../api_impl/mocks/api_impl_mock.gen.go | 26 ++++---- .../manager/api_impl/mocks/varrepl.gen.go | 14 ++-- internal/manager/api_impl/support_test.go | 4 +- internal/manager/api_impl/varrepl.go | 31 +++------ internal/manager/config/service.go | 4 +- internal/manager/config/variables.go | 66 +++++++++++++++++++ 8 files changed, 111 insertions(+), 68 deletions(-) diff --git a/internal/manager/api_impl/meta.go b/internal/manager/api_impl/meta.go index f6f41043..8f597afd 100644 --- a/internal/manager/api_impl/meta.go +++ b/internal/manager/api_impl/meta.go @@ -71,18 +71,12 @@ func (f *Flamenco) GetVariables(e echo.Context, audience api.ManagerVariableAudi func (f *Flamenco) GetSharedStorage(e echo.Context, audience api.ManagerVariableAudience, platform string) error { location := f.config.EffectiveStoragePath() - - feeder := make(chan string, 1) - receiver := make(chan string, 1) - - feeder <- location - close(feeder) - f.config.ExpandVariables(feeder, receiver, config.VariableAudience(audience), config.VariablePlatform(platform)) + varExpand := f.config.NewVariableExpander(config.VariableAudience(audience), config.VariablePlatform(platform)) return e.JSON(http.StatusOK, api.SharedStorageLocation{ Audience: audience, Platform: platform, - Location: <-receiver, + Location: varExpand.Expand(location), ShamanEnabled: f.isShamanEnabled(), }) } diff --git a/internal/manager/api_impl/meta_test.go b/internal/manager/api_impl/meta_test.go index a067ee36..2a1599ad 100644 --- a/internal/manager/api_impl/meta_test.go +++ b/internal/manager/api_impl/meta_test.go @@ -90,12 +90,10 @@ func TestGetSharedStorage(t *testing.T) { mf.config.EXPECT().EffectiveStoragePath().Return(`S:\storage\flamenco`).AnyTimes() { // Test user client on Linux. + // Defer to the actual ExpandVariables() implementation of the above config. mf.config.EXPECT(). - ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceUsers, config.VariablePlatformLinux). - Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) { - // Defer to the actual ExpandVariables() implementation of the above config. - conf.ExpandVariables(inputChannel, outputChannel, audience, platform) - }) + NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformLinux). + DoAndReturn(conf.NewVariableExpander) mf.shaman.EXPECT().IsEnabled().Return(false) echoCtx := mf.prepareMockedRequest(nil) @@ -109,12 +107,10 @@ func TestGetSharedStorage(t *testing.T) { } { // Test worker client on Linux with Shaman enabled. + // Defer to the actual ExpandVariables() implementation of the above config. mf.config.EXPECT(). - ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceWorkers, config.VariablePlatformLinux). - Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) { - // Defer to the actual ExpandVariables() implementation of the above config. - conf.ExpandVariables(inputChannel, outputChannel, audience, platform) - }) + NewVariableExpander(config.VariableAudienceWorkers, config.VariablePlatformLinux). + DoAndReturn(conf.NewVariableExpander) mf.shaman.EXPECT().IsEnabled().Return(true) echoCtx := mf.prepareMockedRequest(nil) @@ -129,12 +125,10 @@ func TestGetSharedStorage(t *testing.T) { } { // Test user client on Windows. + // Defer to the actual ExpandVariables() implementation of the above config. mf.config.EXPECT(). - ExpandVariables(gomock.Any(), gomock.Any(), config.VariableAudienceUsers, config.VariablePlatformWindows). - Do(func(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) { - // Defer to the actual ExpandVariables() implementation of the above config. - conf.ExpandVariables(inputChannel, outputChannel, audience, platform) - }) + NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformWindows). + DoAndReturn(conf.NewVariableExpander) mf.shaman.EXPECT().IsEnabled().Return(false) echoCtx := mf.prepareMockedRequest(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 0b45ba28..08b7e319 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -855,18 +855,6 @@ func (mr *MockConfigServiceMockRecorder) EffectiveStoragePath() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EffectiveStoragePath", reflect.TypeOf((*MockConfigService)(nil).EffectiveStoragePath)) } -// ExpandVariables mocks base method. -func (m *MockConfigService) ExpandVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "ExpandVariables", arg0, arg1, arg2, arg3) -} - -// ExpandVariables indicates an expected call of ExpandVariables. -func (mr *MockConfigServiceMockRecorder) ExpandVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpandVariables", reflect.TypeOf((*MockConfigService)(nil).ExpandVariables), arg0, arg1, arg2, arg3) -} - // ForceFirstRun mocks base method. func (m *MockConfigService) ForceFirstRun() { m.ctrl.T.Helper() @@ -908,6 +896,20 @@ func (mr *MockConfigServiceMockRecorder) IsFirstRun() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsFirstRun", reflect.TypeOf((*MockConfigService)(nil).IsFirstRun)) } +// NewVariableExpander mocks base method. +func (m *MockConfigService) NewVariableExpander(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.VariableExpander { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NewVariableExpander", arg0, arg1) + ret0, _ := ret[0].(*config.VariableExpander) + return ret0 +} + +// NewVariableExpander indicates an expected call of NewVariableExpander. +func (mr *MockConfigServiceMockRecorder) NewVariableExpander(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableExpander", reflect.TypeOf((*MockConfigService)(nil).NewVariableExpander), arg0, arg1) +} + // NewVariableToValueConverter mocks base method. func (m *MockConfigService) NewVariableToValueConverter(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.ValueToVariableReplacer { m.ctrl.T.Helper() diff --git a/internal/manager/api_impl/mocks/varrepl.gen.go b/internal/manager/api_impl/mocks/varrepl.gen.go index 61d1dcdb..7a9f3aa9 100644 --- a/internal/manager/api_impl/mocks/varrepl.gen.go +++ b/internal/manager/api_impl/mocks/varrepl.gen.go @@ -34,16 +34,18 @@ func (m *MockVariableReplacer) EXPECT() *MockVariableReplacerMockRecorder { return m.recorder } -// ExpandVariables mocks base method. -func (m *MockVariableReplacer) ExpandVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) { +// NewVariableExpander mocks base method. +func (m *MockVariableReplacer) NewVariableExpander(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.VariableExpander { m.ctrl.T.Helper() - m.ctrl.Call(m, "ExpandVariables", arg0, arg1, arg2, arg3) + ret := m.ctrl.Call(m, "NewVariableExpander", arg0, arg1) + ret0, _ := ret[0].(*config.VariableExpander) + return ret0 } -// ExpandVariables indicates an expected call of ExpandVariables. -func (mr *MockVariableReplacerMockRecorder) ExpandVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { +// NewVariableExpander indicates an expected call of NewVariableExpander. +func (mr *MockVariableReplacerMockRecorder) NewVariableExpander(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpandVariables", reflect.TypeOf((*MockVariableReplacer)(nil).ExpandVariables), arg0, arg1, arg2, arg3) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableExpander", reflect.TypeOf((*MockVariableReplacer)(nil).NewVariableExpander), arg0, arg1) } // NewVariableToValueConverter mocks base method. diff --git a/internal/manager/api_impl/support_test.go b/internal/manager/api_impl/support_test.go index a5a72318..972d8c8e 100644 --- a/internal/manager/api_impl/support_test.go +++ b/internal/manager/api_impl/support_test.go @@ -101,8 +101,8 @@ func (mf *mockedFlamenco) expectExpandVariables( // Defer the mocked call to the fake configuration. return mf.config.EXPECT(). - ExpandVariables(gomock.Any(), gomock.Any(), expectAudience, expectPlatform). - DoAndReturn(c.ExpandVariables) + NewVariableExpander(expectAudience, expectPlatform). + DoAndReturn(c.NewVariableExpander) } func (mf *mockedFlamenco) expectConvertTwoWayVariables( diff --git a/internal/manager/api_impl/varrepl.go b/internal/manager/api_impl/varrepl.go index fa56640b..f70cedfe 100644 --- a/internal/manager/api_impl/varrepl.go +++ b/internal/manager/api_impl/varrepl.go @@ -3,8 +3,6 @@ package api_impl // SPDX-License-Identifier: GPL-3.0-or-later import ( - "sync" - "git.blender.org/flamenco/internal/manager/config" "git.blender.org/flamenco/internal/manager/persistence" "git.blender.org/flamenco/pkg/api" @@ -12,36 +10,28 @@ import ( //go:generate go run github.com/golang/mock/mockgen -destination mocks/varrepl.gen.go -package mocks git.blender.org/flamenco/internal/manager/api_impl VariableReplacer type VariableReplacer interface { - ExpandVariables(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) + NewVariableExpander(audience config.VariableAudience, platform config.VariablePlatform) *config.VariableExpander ResolveVariables(audience config.VariableAudience, platform config.VariablePlatform) map[string]config.ResolvedVariable NewVariableToValueConverter(audience config.VariableAudience, platform config.VariablePlatform) *config.ValueToVariableReplacer } // replaceTaskVariables performs variable replacement for worker tasks. func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, worker persistence.Worker) api.AssignedTask { - feeder := make(chan string, 1) - receiver := make(chan string, 1) - - wg := sync.WaitGroup{} - wg.Add(1) - go func() { - defer wg.Done() - replacer.ExpandVariables(feeder, receiver, - config.VariableAudienceWorkers, config.VariablePlatform(worker.Platform)) - }() + varExpander := replacer.NewVariableExpander( + config.VariableAudienceWorkers, + config.VariablePlatform(worker.Platform), + ) for cmdIndex, cmd := range task.Commands { for key, value := range cmd.Parameters { switch v := value.(type) { case string: - feeder <- v - task.Commands[cmdIndex].Parameters[key] = <-receiver + task.Commands[cmdIndex].Parameters[key] = varExpander.Expand(v) case []string: replaced := make([]string, len(v)) for idx := range v { - feeder <- v[idx] - replaced[idx] = <-receiver + replaced[idx] = varExpander.Expand(v[idx]) } task.Commands[cmdIndex].Parameters[key] = replaced @@ -50,8 +40,7 @@ func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, work for idx := range v { switch itemValue := v[idx].(type) { case string: - feeder <- itemValue - replaced[idx] = <-receiver + replaced[idx] = varExpander.Expand(itemValue) default: replaced[idx] = itemValue } @@ -64,10 +53,6 @@ func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, work } } - close(feeder) - wg.Wait() - close(receiver) - return task } diff --git a/internal/manager/config/service.go b/internal/manager/config/service.go index d191eaef..0ce13489 100644 --- a/internal/manager/config/service.go +++ b/internal/manager/config/service.go @@ -71,8 +71,8 @@ func (s *Service) Save() error { } // Expose some functions of Conf here, for easier mocking of functionality via interfaces. -func (s *Service) ExpandVariables(inputChannel <-chan string, outputChannel chan<- string, audience VariableAudience, platform VariablePlatform) { - s.config.ExpandVariables(inputChannel, outputChannel, audience, platform) +func (s *Service) NewVariableExpander(audience VariableAudience, platform VariablePlatform) *VariableExpander { + return s.config.NewVariableExpander(audience, platform) } func (s *Service) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer { return s.config.NewVariableToValueConverter(audience, platform) diff --git a/internal/manager/config/variables.go b/internal/manager/config/variables.go index 51a694b9..f347c027 100644 --- a/internal/manager/config/variables.go +++ b/internal/manager/config/variables.go @@ -12,6 +12,14 @@ type ValueToVariableReplacer struct { twoWayVars map[string]string // Mapping from variable name to value. } +// VariableExpander expands variables and applies two-way variable replacement to the values. +type VariableExpander struct { + oneWayVars map[string]string // Mapping from variable name to value. + managerTwoWayVars map[string]string // Mapping from variable name to value for the Manager platform. + targetTwoWayVars map[string]string // Mapping from variable name to value for the target platform. + targetPlatform VariablePlatform +} + // NewVariableToValueConverter returns a ValueToVariableReplacer for the given audience & platform. func (c *Conf) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer { // Get the variables for the given audience & platform. @@ -29,6 +37,25 @@ func (c *Conf) NewVariableToValueConverter(audience VariableAudience, platform V } } +// NewVariableExpander returns a new VariableExpander for the given audience & platform. +func (c *Conf) NewVariableExpander(audience VariableAudience, platform VariablePlatform) *VariableExpander { + // Get the variables for the given audience & platform. + varsForPlatform := c.getVariables(audience, platform) + if len(varsForPlatform) == 0 { + log.Warn(). + Str("audience", string(audience)). + Str("platform", string(platform)). + Msg("no variables defined for this platform given this audience") + } + + return &VariableExpander{ + oneWayVars: varsForPlatform, + managerTwoWayVars: c.GetTwoWayVariables(audience, c.currentGOOS), + targetTwoWayVars: c.GetTwoWayVariables(audience, platform), + targetPlatform: platform, + } +} + // ValueToVariableReplacer replaces any variable values it recognises in // valueToConvert to the actual variable. For example, `/path/to/file.blend` can // be changed to `{my_storage}/file.blend`. @@ -72,3 +99,42 @@ func isValueMatch(valueToMatch, variableValue string) bool { return false } + +// Replace converts "{variable name}" to the value that belongs to the audience and platform. +func (ve *VariableExpander) Expand(valueToExpand string) string { + expanded := valueToExpand + + // Expand variables from {varname} to their value for the target platform. + for varname, varvalue := range ve.oneWayVars { + placeholder := fmt.Sprintf("{%s}", varname) + expanded = strings.Replace(expanded, placeholder, varvalue, -1) + } + + // Go through the two-way variables, to make sure that the result of + // expanding variables gets the two-way variables applied as well. This is + // necessary to make implicitly-defined variable, which are only defined for + // the Manager's platform, usable for the target platform. + // + // Practically, this replaces "value for the Manager platform" with "value + // for the target platform". + isPathValue := false + for varname, managerValue := range ve.managerTwoWayVars { + targetValue, ok := ve.targetTwoWayVars[varname] + if !ok { + continue + } + if !isValueMatch(expanded, managerValue) { + continue + } + expanded = targetValue + expanded[len(managerValue):] + + // Since two-way variables are meant for path replacement, we know this + // should be a path. + isPathValue = true + } + + if isPathValue { + expanded = crosspath.ToPlatform(expanded, string(ve.targetPlatform)) + } + return expanded +}