Manager: simplify value-to-variable replacement

Simplify the code for the two-way variables' value-to-variable replacement.

Instead of using a goroutine and two channels, use a separate struct and
call a function on that directly.

No functional changes.

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()

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()

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).
+		NewVariableToValueConverter(expectAudience, expectPlatform).
-		DoAndReturn(c.ConvertTwoWayVariables)
+		DoAndReturn(c.NewVariableToValueConverter)
 }
 
 // prepareMockedJSONRequest returns an `echo.Context` that has a JSON request body attached to it.

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)
+	valueToVariable := replacer.NewVariableToValueConverter(
-	receiver := make(chan string, 1)
+		config.VariableAudienceWorkers,
-
+		config.VariablePlatform(job.SubmitterPlatform),
-	wg := sync.WaitGroup{}
+	)
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		replacer.ConvertTwoWayVariables(feeder, receiver,
-			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
+			newValue := valueToVariable.Replace(stringValue)
-			job.Settings.AdditionalProperties[settingKey] = <-receiver
+			job.Settings.AdditionalProperties[settingKey] = newValue
 		}
 	}
 	if job.Metadata != nil {
 		for metaKey, metaValue := range job.Metadata.AdditionalProperties {
-			feeder <- metaValue
+			newValue := valueToVariable.Replace(metaValue)
-			job.Metadata.AdditionalProperties[metaKey] = <-receiver
+			job.Metadata.AdditionalProperties[metaKey] = newValue
 		}
 	}
-
-	close(feeder)
-	wg.Wait()
-	close(receiver)
 }

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

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)
-}

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) {
+func (s *Service) NewVariableToValueConverter(audience VariableAudience, platform VariablePlatform) *ValueToVariableReplacer {
-	s.config.ConvertTwoWayVariables(inputChannel, outputChannel, audience, platform)
+	return s.config.NewVariableToValueConverter(audience, platform)
 }
 func (s *Service) ResolveVariables(audience VariableAudience, platform VariablePlatform) map[string]ResolvedVariable {
 	return s.config.ResolveVariables(audience, platform)

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
+}

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`))
+}