Fix variant expansion for bool config fields

Summary:
Refactored bool config fields to use a new `config.Flag` type that keeps
track of whether it was set by unmarshalled data or merged from another
instance, fixing the behavior of these fields when overwritten by
variants.

Fixes T166353

Test Plan: Run config unit tests (`cd config; go test`)

Reviewers: thcipriani, Joe, hashar, mobrovac, mmodell, #release-engineering-team

Reviewed By: mobrovac

Tags: #release-engineering-team

Maniphest Tasks: T166353

Differential Revision: https://phabricator.wikimedia.org/D680

4 files changed, 66 insertions, 16 deletions

diff --git a/config/common.go b/config/common.go
index 6f3b97a..31916dd 100644
--- a/config/common.go
+++ b/config/common.go
@@ -9,7 +9,7 @@ type CommonConfig struct {
 	Apt AptConfig `yaml:"apt"`
 	Npm NpmConfig `yaml:"npm"`
 	Runs RunsConfig `yaml:"runs"`
-	SharedVolume bool `yaml:"sharedvolume"`
+	SharedVolume Flag `yaml:"sharedvolume"`
 	EntryPoint []string `yaml:"entrypoint"`
 }
 
@@ -21,8 +21,7 @@ func (cc1 *CommonConfig) Merge(cc2 CommonConfig) {
 	cc1.Apt.Merge(cc2.Apt)
 	cc1.Npm.Merge(cc2.Npm)
 	cc1.Runs.Merge(cc2.Runs)
-
-	cc1.SharedVolume = cc1.SharedVolume || cc2.SharedVolume
+	cc1.SharedVolume.Merge(cc2.SharedVolume)
 
 	if len(cc1.EntryPoint) < 1 {
 		cc1.EntryPoint = cc2.EntryPoint
diff --git a/config/flag.go b/config/flag.go
new file mode 100644
index 0000000..aa4cc68
--- /dev/null
+++ b/config/flag.go
@@ -0,0 +1,23 @@
+package config
+
+type Flag struct {
+	True bool
+	set bool
+}
+
+func (flag *Flag) UnmarshalYAML(unmarshal func(interface {}) error) error {
+	if err := unmarshal(&flag.True); err != nil {
+		return err
+	}
+
+	flag.set = true
+
+	return nil
+}
+
+func (flag *Flag) Merge(flag2 Flag) {
+	if flag2.set {
+		flag.True = flag2.True
+		flag.set = true
+	}
+}
diff --git a/config/flag_test.go b/config/flag_test.go
new file mode 100644
index 0000000..39aa254
--- /dev/null
+++ b/config/flag_test.go
@@ -0,0 +1,31 @@
+package config_test
+
+import (
+	"testing"
+	"gopkg.in/stretchr/testify.v1/assert"
+
+	"phabricator.wikimedia.org/source/blubber.git/config"
+)
+
+const yaml = `---
+npm: { install: true }
+sharedvolume: false
+
+variants:
+  development:
+    sharedvolume: true
+    npm: { install: false }
+`
+
+func TestFlagOverwrite(t *testing.T) {
+	cfg, err := config.ReadConfig([]byte(yaml))
+
+	assert.Nil(t, err)
+
+	variant, err := config.ExpandVariant(cfg, "development")
+
+	assert.Nil(t, err)
+
+	assert.False(t, variant.Npm.Install.True)
+	assert.True(t, variant.SharedVolume.True)
+}
diff --git a/config/npm.go b/config/npm.go
index f02df54..91cfc26 100644
--- a/config/npm.go
+++ b/config/npm.go
@@ -1,20 +1,19 @@
 package config
 
 import (
-	"bytes"
 	"path"
 	"phabricator.wikimedia.org/source/blubber.git/build"
 )
 
-const TempNpmInstallDir = "/tmp/node-deps/"
+const tempNpmInstallDir = "/tmp/node-deps/"
 
 type NpmConfig struct {
-	Install bool `yaml:"install"`
+	Install Flag `yaml:"install"`
 	Env string `yaml:"env"`
 }
 
 func (npm *NpmConfig) Merge(npm2 NpmConfig) {
-	npm.Install = npm.Install || npm2.Install
+	npm.Install.Merge(npm2.Install)
 
 	if npm2.Env != "" {
 		npm.Env = npm2.Env
@@ -22,25 +21,23 @@ func (npm *NpmConfig) Merge(npm2 NpmConfig) {
 }
 
 func (npm NpmConfig) InstructionsForPhase(phase build.Phase) []build.Instruction{
-	if npm.Install {
+	if npm.Install.True {
 		switch phase {
 		case build.PhasePreInstall:
-			npmCmd := new(bytes.Buffer)
-
-			npmCmd.WriteString("npm install")
+			npmCmd := "npm install"
 
 			if npm.Env == "production" {
-				npmCmd.WriteString(" --production && npm dedupe")
+				npmCmd += " --production && npm dedupe"
 			}
 
 			return []build.Instruction{
-				{build.Run, []string{"mkdir -p ", TempNpmInstallDir}},
-				{build.Copy, []string{"package.json", TempNpmInstallDir}},
-				{build.Run, []string{"cd ", TempNpmInstallDir, " && ", npmCmd.String()}},
+				{build.Run, []string{"mkdir -p ", tempNpmInstallDir}},
+				{build.Copy, []string{"package.json", tempNpmInstallDir}},
+				{build.Run, []string{"cd ", tempNpmInstallDir, " && ", npmCmd}},
 			}
 		case build.PhasePostInstall:
 			return []build.Instruction{
-				{build.Run, []string{"mv ", path.Join(TempNpmInstallDir, "node_modules"), " ./"}},
+				{build.Run, []string{"mv ", path.Join(tempNpmInstallDir, "node_modules"), " ./"}},
 			}
 		}
 	}