From bedf7869f8d078afd7e297b706e671524a2308dc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Thu, 17 Mar 2022 11:33:41 +0100
Subject: [PATCH] Manager: replace "direction=twoway" with "is_twoway=true" in
 config

A boolean provides less context to the setting, so it's not as easy to
understand. However, in this case the simple case will have `is_twoway=false` and be ommitted from the configuration file. This makes the simple case even simpler.
---
 internal/manager/config/defaults.go      |  6 ++----
 internal/manager/config/settings.go      | 18 ++----------------
 internal/manager/config/settings_test.go | 15 ++++-----------
 3 files changed, 8 insertions(+), 31 deletions(-)

diff --git a/internal/manager/config/defaults.go b/internal/manager/config/defaults.go
index 9e7ebcdc..c11ce894 100644
--- a/internal/manager/config/defaults.go
+++ b/internal/manager/config/defaults.go
@@ -51,7 +51,6 @@ var defaultConfig = Conf{
 	Variables: map[string]Variable{
 		// The default commands assume that the executables are available on $PATH.
 		"blender": {
-			Direction: "oneway",
 			Values: VariableValues{
 				VariableValue{Platform: "linux", Value: "blender --factory-startup --background"},
 				VariableValue{Platform: "windows", Value: "blender.exe --factory-startup --background"},
@@ -59,7 +58,6 @@ var defaultConfig = Conf{
 			},
 		},
 		"ffmpeg": {
-			Direction: "oneway",
 			Values: VariableValues{
 				VariableValue{Platform: "linux", Value: "ffmpeg"},
 				VariableValue{Platform: "windows", Value: "ffmpeg.exe"},
@@ -68,7 +66,7 @@ var defaultConfig = Conf{
 		},
 		// TODO: determine useful defaults for these.
 		// "job_storage": {
-		// 	Direction: "twoway",
+		// 	IsTwoWay: true,
 		// 	Values: VariableValues{
 		// 		VariableValue{Platform: "linux", Value: "/shared/flamenco/jobs"},
 		// 		VariableValue{Platform: "windows", Value: "S:/flamenco/jobs"},
@@ -76,7 +74,7 @@ var defaultConfig = Conf{
 		// 	},
 		// },
 		// "render": {
-		// 	Direction: "twoway",
+		// 	IsTwoWay: true,
 		// 	Values: VariableValues{
 		// 		VariableValue{Platform: "linux", Value: "/shared/flamenco/render"},
 		// 		VariableValue{Platform: "windows", Value: "S:/flamenco/render"},
diff --git a/internal/manager/config/settings.go b/internal/manager/config/settings.go
index 4cb93b38..2cb2b00c 100644
--- a/internal/manager/config/settings.go
+++ b/internal/manager/config/settings.go
@@ -145,8 +145,7 @@ type Conf struct {
 
 // Variable defines a configuration variable.
 type Variable struct {
-	// Either "oneway" or "twoway"
-	Direction string `yaml:"direction" json:"direction"`
+	IsTwoWay bool `yaml:"is_twoway,omitempty" json:"is_twoway,omitempty"`
 	// Mapping from variable value to audience/platform definition.
 	Values VariableValues `yaml:"values" json:"values"`
 }
@@ -285,7 +284,7 @@ func (c *Conf) constructVariableLookupTable(logLevel zerolog.Level) {
 			// Two-way values should not end in path separator.
 			// Given a variable 'apps' with value '/path/to/apps',
 			// '/path/to/apps/blender' should be remapped to '{apps}/blender'.
-			if variable.Direction == "twoway" {
+			if variable.IsTwoWay {
 				if strings.Contains(value.Value, "\\") {
 					log.Warn().
 						Str("variable", name).
@@ -346,20 +345,7 @@ func (c *Conf) ExpandVariables(valueToExpand string, audience VariableAudience,
 // checkVariables performs some basic checks on variable definitions.
 // All errors are logged, not returned.
 func (c *Conf) checkVariables() {
-	directionNames := []string{"oneway", "twoway"}
-	validDirections := map[string]bool{}
-	for _, direction := range directionNames {
-		validDirections[direction] = true
-	}
-
 	for name, variable := range c.Variables {
-		if !validDirections[variable.Direction] {
-			log.Error().
-				Str("name", name).
-				Str("direction", variable.Direction).
-				Strs("validChoices", directionNames).
-				Msg("variable has invalid direction")
-		}
 		for valueIndex, value := range variable.Values {
 			// No platforms at all.
 			if value.Platform == "" && len(value.Platforms) == 0 {
diff --git a/internal/manager/config/settings_test.go b/internal/manager/config/settings_test.go
index a1bc9c1a..6073166b 100644
--- a/internal/manager/config/settings_test.go
+++ b/internal/manager/config/settings_test.go
@@ -17,19 +17,9 @@ func TestDefaultSettings(t *testing.T) {
 	assert.Equal(t, defaultConfig.TaskLogsPath, config.TaskLogsPath)
 	assert.Equal(t, defaultConfig.DatabaseDSN, config.DatabaseDSN)
 
-	assert.Contains(t, config.Variables, "job_storage")
-	assert.Contains(t, config.Variables, "render")
-	assert.Equal(t, "oneway", config.Variables["ffmpeg"].Direction)
+	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)
-
-	linuxPVars, ok := config.VariablesLookup["workers"]["linux"]
-	assert.True(t, ok, "workers/linux should have variables: %v", config.VariablesLookup)
-	assert.Equal(t, "/shared/flamenco/jobs", linuxPVars["job_storage"])
-
-	winPVars, ok := config.VariablesLookup["users"]["windows"]
-	assert.True(t, ok)
-	assert.Equal(t, "S:/flamenco/jobs", winPVars["job_storage"])
 }
 
 func TestVariableValidation(t *testing.T) {
@@ -47,3 +37,6 @@ func TestVariableValidation(t *testing.T) {
 	assert.Equal(t, c.Variables["blender"].Values[0].Value, "/path/to/blender")
 	assert.Equal(t, c.Variables["blender"].Values[1].Value, "/valid/path/blender")
 }
+
+// TODO: Test two-way variables. Even though they're not currently in the
+// default configuration, they should work.