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 6817cfce..0b45ba28 100644 --- a/internal/manager/api_impl/mocks/api_impl_mock.gen.go +++ b/internal/manager/api_impl/mocks/api_impl_mock.gen.go @@ -841,18 +841,6 @@ func (m *MockConfigService) EXPECT() *MockConfigServiceMockRecorder { return m.recorder } -// ConvertTwoWayVariables mocks base method. -func (m *MockConfigService) ConvertTwoWayVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "ConvertTwoWayVariables", arg0, arg1, arg2, arg3) -} - -// ConvertTwoWayVariables indicates an expected call of ConvertTwoWayVariables. -func (mr *MockConfigServiceMockRecorder) ConvertTwoWayVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConvertTwoWayVariables", reflect.TypeOf((*MockConfigService)(nil).ConvertTwoWayVariables), arg0, arg1, arg2, arg3) -} - // EffectiveStoragePath mocks base method. func (m *MockConfigService) EffectiveStoragePath() string { m.ctrl.T.Helper() @@ -920,6 +908,20 @@ func (mr *MockConfigServiceMockRecorder) IsFirstRun() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsFirstRun", reflect.TypeOf((*MockConfigService)(nil).IsFirstRun)) } +// NewVariableToValueConverter mocks base method. +func (m *MockConfigService) NewVariableToValueConverter(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.ValueToVariableReplacer { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NewVariableToValueConverter", arg0, arg1) + ret0, _ := ret[0].(*config.ValueToVariableReplacer) + return ret0 +} + +// NewVariableToValueConverter indicates an expected call of NewVariableToValueConverter. +func (mr *MockConfigServiceMockRecorder) NewVariableToValueConverter(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableToValueConverter", reflect.TypeOf((*MockConfigService)(nil).NewVariableToValueConverter), arg0, arg1) +} + // ResolveVariables mocks base method. func (m *MockConfigService) ResolveVariables(arg0 config.VariableAudience, arg1 config.VariablePlatform) map[string]config.ResolvedVariable { 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 6b7da3c9..61d1dcdb 100644 --- a/internal/manager/api_impl/mocks/varrepl.gen.go +++ b/internal/manager/api_impl/mocks/varrepl.gen.go @@ -34,18 +34,6 @@ func (m *MockVariableReplacer) EXPECT() *MockVariableReplacerMockRecorder { return m.recorder } -// ConvertTwoWayVariables mocks base method. -func (m *MockVariableReplacer) ConvertTwoWayVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) { - m.ctrl.T.Helper() - m.ctrl.Call(m, "ConvertTwoWayVariables", arg0, arg1, arg2, arg3) -} - -// ConvertTwoWayVariables indicates an expected call of ConvertTwoWayVariables. -func (mr *MockVariableReplacerMockRecorder) ConvertTwoWayVariables(arg0, arg1, arg2, arg3 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConvertTwoWayVariables", reflect.TypeOf((*MockVariableReplacer)(nil).ConvertTwoWayVariables), arg0, arg1, arg2, arg3) -} - // ExpandVariables mocks base method. func (m *MockVariableReplacer) ExpandVariables(arg0 <-chan string, arg1 chan<- string, arg2 config.VariableAudience, arg3 config.VariablePlatform) { m.ctrl.T.Helper() @@ -58,6 +46,20 @@ func (mr *MockVariableReplacerMockRecorder) ExpandVariables(arg0, arg1, arg2, ar return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpandVariables", reflect.TypeOf((*MockVariableReplacer)(nil).ExpandVariables), arg0, arg1, arg2, arg3) } +// NewVariableToValueConverter mocks base method. +func (m *MockVariableReplacer) NewVariableToValueConverter(arg0 config.VariableAudience, arg1 config.VariablePlatform) *config.ValueToVariableReplacer { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NewVariableToValueConverter", arg0, arg1) + ret0, _ := ret[0].(*config.ValueToVariableReplacer) + return ret0 +} + +// NewVariableToValueConverter indicates an expected call of NewVariableToValueConverter. +func (mr *MockVariableReplacerMockRecorder) NewVariableToValueConverter(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NewVariableToValueConverter", reflect.TypeOf((*MockVariableReplacer)(nil).NewVariableToValueConverter), arg0, arg1) +} + // ResolveVariables mocks base method. func (m *MockVariableReplacer) ResolveVariables(arg0 config.VariableAudience, arg1 config.VariablePlatform) map[string]config.ResolvedVariable { m.ctrl.T.Helper() diff --git a/internal/manager/api_impl/support_test.go b/internal/manager/api_impl/support_test.go index fb96dbb0..a5a72318 100644 --- a/internal/manager/api_impl/support_test.go +++ b/internal/manager/api_impl/support_test.go @@ -126,8 +126,8 @@ func (mf *mockedFlamenco) expectConvertTwoWayVariables( // Defer the mocked call to the fake configuration. return mf.config.EXPECT(). - ConvertTwoWayVariables(gomock.Any(), gomock.Any(), expectAudience, expectPlatform). - DoAndReturn(c.ConvertTwoWayVariables) + NewVariableToValueConverter(expectAudience, expectPlatform). + DoAndReturn(c.NewVariableToValueConverter) } // prepareMockedJSONRequest returns an `echo.Context` that has a JSON request body attached to it. diff --git a/internal/manager/api_impl/varrepl.go b/internal/manager/api_impl/varrepl.go index 562ce81e..fa56640b 100644 --- a/internal/manager/api_impl/varrepl.go +++ b/internal/manager/api_impl/varrepl.go @@ -14,7 +14,7 @@ import ( type VariableReplacer interface { ExpandVariables(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) ResolveVariables(audience config.VariableAudience, platform config.VariablePlatform) map[string]config.ResolvedVariable - ConvertTwoWayVariables(inputChannel <-chan string, outputChannel chan<- string, audience config.VariableAudience, platform config.VariablePlatform) + NewVariableToValueConverter(audience config.VariableAudience, platform config.VariablePlatform) *config.ValueToVariableReplacer } // replaceTaskVariables performs variable replacement for worker tasks. @@ -78,16 +78,10 @@ func replaceTaskVariables(replacer VariableReplacer, task api.AssignedTask, work // // NOTE: this updates the job in place. func replaceTwoWayVariables(replacer VariableReplacer, job *api.SubmittedJob) { - feeder := make(chan string, 1) - receiver := make(chan string, 1) - - wg := sync.WaitGroup{} - wg.Add(1) - go func() { - defer wg.Done() - replacer.ConvertTwoWayVariables(feeder, receiver, - config.VariableAudienceWorkers, config.VariablePlatform(job.SubmitterPlatform)) - }() + valueToVariable := replacer.NewVariableToValueConverter( + config.VariableAudienceWorkers, + config.VariablePlatform(job.SubmitterPlatform), + ) // Only replace variables in settings and metadata, not in other job fields. if job.Settings != nil { @@ -96,18 +90,14 @@ func replaceTwoWayVariables(replacer VariableReplacer, job *api.SubmittedJob) { if !ok { continue } - feeder <- stringValue - job.Settings.AdditionalProperties[settingKey] = <-receiver + newValue := valueToVariable.Replace(stringValue) + job.Settings.AdditionalProperties[settingKey] = newValue } } if job.Metadata != nil { for metaKey, metaValue := range job.Metadata.AdditionalProperties { - feeder <- metaValue - job.Metadata.AdditionalProperties[metaKey] = <-receiver + newValue := valueToVariable.Replace(metaValue) + job.Metadata.AdditionalProperties[metaKey] = newValue } } - - close(feeder) - wg.Wait() - close(receiver) } diff --git a/internal/manager/config/config.go b/internal/manager/config/config.go index 66e6cc5d..b280366d 100644 --- a/internal/manager/config/config.go +++ b/internal/manager/config/config.go @@ -472,57 +472,6 @@ func (c *Conf) ExpandVariables(inputChannel <-chan string, outputChannel chan<- } } -// ConvertTwoWayVariables converts the value of a variable with "{variable -// name}", but only for two-way variables. The function iterates over all -// strings provided by the input channel, and sends the expanded result into the -// output channel. It will return when the input channel is closed. -func (c *Conf) ConvertTwoWayVariables(inputChannel <-chan string, outputChannel chan<- string, - audience VariableAudience, platform VariablePlatform) { - - // Get the variables for the given audience & platform. - twoWayVars := c.GetTwoWayVariables(audience, platform) - if len(twoWayVars) == 0 { - log.Debug(). - Str("audience", string(audience)). - Str("platform", string(platform)). - Msg("no two-way variables defined for this platform given this audience") - } - - doValueReplacement := func(valueToConvert string) string { - for varName, varValue := range twoWayVars { - if !isValueMatch(valueToConvert, varValue) { - continue - } - valueToConvert = fmt.Sprintf("{%s}%s", varName, valueToConvert[len(varValue):]) - } - - return valueToConvert - } - - for valueToExpand := range inputChannel { - outputChannel <- doValueReplacement(valueToExpand) - } -} - -// isValueMatch returns whether `valueToMatch` starts with `variableValue`. -// When `variableValue` is a Windows path (with backslash separators), it is -// also tested with forward slashes against `valueToMatch`. -func isValueMatch(valueToMatch, variableValue string) bool { - if strings.HasPrefix(valueToMatch, variableValue) { - return true - } - - // If the variable value has a backslash, assume it is a Windows path. - // Convert it to slash notation just to see if that would provide a - // match. - if strings.ContainsRune(variableValue, '\\') { - slashedValue := crosspath.ToSlash(variableValue) - return strings.HasPrefix(valueToMatch, slashedValue) - } - - return false -} - // getVariables returns the variable values for this (audience, platform) combination. // If no variables are found, just returns an empty map. If a value is defined // for both the "all" platform and specifically the given platform, the specific diff --git a/internal/manager/config/config_test.go b/internal/manager/config/config_test.go index 3838dde5..c6c18203 100644 --- a/internal/manager/config/config_test.go +++ b/internal/manager/config/config_test.go @@ -22,30 +22,3 @@ func TestVariablesWithBackslashes(t *testing.T) { assert.Equal(t, `C:\Downloads\blender-1.0\`, vars["single-backslash-trailing"]["blender"]) assert.Equal(t, `F:\`, vars["single-backslash-drive-only"]["blender"]) } - -func TestReplaceTwowayVariables(t *testing.T) { - c := DefaultConfig(func(c *Conf) { - c.Variables["shared"] = Variable{ - IsTwoWay: true, - Values: []VariableValue{ - {Value: "/shared/flamenco", Platform: VariablePlatformLinux}, - {Value: `Y:\shared\flamenco`, Platform: VariablePlatformWindows}, - }, - } - }) - - feeder := make(chan string, 2) - receiver := make(chan string, 2) - - feeder <- `Y:\shared\flamenco\shot\file.blend` - - // This is the real reason for this test: forward slashes in the path should - // still be matched to the backslashes in the variable value. - feeder <- `Y:/shared/flamenco/shot/file.blend` - close(feeder) - - c.ConvertTwoWayVariables(feeder, receiver, VariableAudienceUsers, VariablePlatformWindows) - - assert.Equal(t, `{shared}\shot\file.blend`, <-receiver) - assert.Equal(t, `{shared}/shot/file.blend`, <-receiver) -} diff --git a/internal/manager/config/service.go b/internal/manager/config/service.go index c58cafee..d191eaef 100644 --- a/internal/manager/config/service.go +++ b/internal/manager/config/service.go @@ -70,12 +70,12 @@ func (s *Service) Save() error { return nil } -// Expose some functions on Conf here, for easier mocking of functionality via interfaces. +// 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) ConvertTwoWayVariables(inputChannel <-chan string, outputChannel chan<- string, audience VariableAudience, platform VariablePlatform) { - s.config.ConvertTwoWayVariables(inputChannel, outputChannel, audience, platform) +func (s *Service) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer { + return s.config.NewVariableToValueConverter(audience, platform) } func (s *Service) ResolveVariables(audience VariableAudience, platform VariablePlatform) map[string]ResolvedVariable { return s.config.ResolveVariables(audience, platform) diff --git a/internal/manager/config/variables.go b/internal/manager/config/variables.go new file mode 100644 index 00000000..51a694b9 --- /dev/null +++ b/internal/manager/config/variables.go @@ -0,0 +1,74 @@ +package config + +import ( + "fmt" + "strings" + + "git.blender.org/flamenco/pkg/crosspath" + "github.com/rs/zerolog/log" +) + +type ValueToVariableReplacer struct { + twoWayVars map[string]string // Mapping from variable name to value. +} + +// 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. + twoWayVars := c.GetTwoWayVariables(audience, platform) + + if len(twoWayVars) == 0 { + log.Debug(). + Str("audience", string(audience)). + Str("platform", string(platform)). + Msg("no two-way variables defined for this platform given this audience") + } + + return &ValueToVariableReplacer{ + twoWayVars: twoWayVars, + } +} + +// 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`. +func (vvc *ValueToVariableReplacer) Replace(valueToConvert string) string { + result := valueToConvert + + for varName, varValue := range vvc.twoWayVars { + if !isValueMatch(result, varValue) { + continue + } + result = vvc.join(varName, result[len(varValue):]) + } + + log.Debug(). + Str("from", valueToConvert). + Str("to", result). + Msg("first step of two-way variable replacement") + + return result +} + +func (vvc *ValueToVariableReplacer) join(varName, value string) string { + return fmt.Sprintf("{%s}%s", varName, value) +} + +// isValueMatch returns whether `valueToMatch` starts with `variableValue`. +// When `variableValue` is a Windows path (with backslash separators), it is +// also tested with forward slashes against `valueToMatch`. +func isValueMatch(valueToMatch, variableValue string) bool { + if strings.HasPrefix(valueToMatch, variableValue) { + return true + } + + // If the variable value has a backslash, assume it is a Windows path. + // Convert it to slash notation just to see if that would provide a + // match. + if strings.ContainsRune(variableValue, '\\') { + slashedValue := crosspath.ToSlash(variableValue) + return strings.HasPrefix(valueToMatch, slashedValue) + } + + return false +} diff --git a/internal/manager/config/variables_test.go b/internal/manager/config/variables_test.go new file mode 100644 index 00000000..c6c1e2a6 --- /dev/null +++ b/internal/manager/config/variables_test.go @@ -0,0 +1,26 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestReplaceTwowayVariables(t *testing.T) { + c := DefaultConfig(func(c *Conf) { + c.Variables["shared"] = Variable{ + IsTwoWay: true, + Values: []VariableValue{ + {Value: "/shared/flamenco", Platform: VariablePlatformLinux}, + {Value: `Y:\shared\flamenco`, Platform: VariablePlatformWindows}, + }, + } + }) + + replacer := c.NewVariableToValueConverter(VariableAudienceUsers, VariablePlatformWindows) + + // This is the real reason for this test: forward slashes in the path should + // still be matched to the backslashes in the variable value. + assert.Equal(t, `{shared}\shot\file.blend`, replacer.Replace(`Y:\shared\flamenco\shot\file.blend`)) + assert.Equal(t, `{shared}/shot/file.blend`, replacer.Replace(`Y:/shared/flamenco/shot/file.blend`)) +}