Use real types for build instructions

Summary:
Refactored build instructions to use concrete types and
`build.Instruction` as an interface instead of relying on a simple enum
and arbitrary string arguments. The formal types result in:

 1. Clearer internal data structures
 2. Partial compilation and proper argument quoting for all instructions
    moved into the common `build` package
 3. Higher order instructions like `build.RunAll` that easily reduce to
    compiler specific output

Test Plan: Run `arc unit` or `go test ./...`

Reviewers: thcipriani, mmodell, #release-engineering-team

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

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

6 files changed, 66 insertions, 85 deletions

diff --git a/config/apt.go b/config/apt.go
index 2382bbf..37a5c3e 100644
--- a/config/apt.go
+++ b/config/apt.go
@@ -1,8 +1,6 @@
 package config
 
 import (
-	"strings"
-
 	"phabricator.wikimedia.org/source/blubber.git/build"
 )
 
@@ -19,10 +17,10 @@ func (apt AptConfig) InstructionsForPhase(phase build.Phase) []build.Instruction
 		switch phase {
 		case build.PhasePrivileged:
 			return []build.Instruction{
-				{build.Run, []string{
-					"apt-get update && apt-get install -y ",
-					strings.Join(apt.Packages, " "),
-					" && rm -rf /var/lib/apt/lists/*",
+				build.RunAll{[]build.Run{
+					{"apt-get update", []string{}},
+					{"apt-get install -y", apt.Packages},
+					{"rm -rf /var/lib/apt/lists/*", []string{}},
 				}},
 			}
 		}
diff --git a/config/apt_test.go b/config/apt_test.go
index 3801231..9e0e08c 100644
--- a/config/apt_test.go
+++ b/config/apt_test.go
@@ -38,10 +38,10 @@ func TestAptConfigInstructions(t *testing.T) {
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{
-					"apt-get update && apt-get install -y ",
-					"libfoo libbar",
-					" && rm -rf /var/lib/apt/lists/*",
+				build.RunAll{[]build.Run{
+					{"apt-get update", []string{}},
+					{"apt-get install -y", []string{"libfoo", "libbar"}},
+					{"rm -rf /var/lib/apt/lists/*", []string{}},
 				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePrivileged),
diff --git a/config/npm.go b/config/npm.go
index f263d24..26fd0ad 100644
--- a/config/npm.go
+++ b/config/npm.go
@@ -24,20 +24,26 @@ func (npm NpmConfig) InstructionsForPhase(phase build.Phase) []build.Instruction
 	if npm.Install.True {
 		switch phase {
 		case build.PhasePreInstall:
-			npmCmd := "npm install"
+			npmInstall := build.RunAll{[]build.Run{
+				{"cd", []string{tempNpmInstallDir}},
+				{"npm install", []string{}},
+			}}
 
 			if npm.Env == "production" {
-				npmCmd += " --production && npm dedupe"
+				npmInstall.Runs[1].Arguments = []string{"--production"}
+				npmInstall.Runs = append(npmInstall.Runs,
+					build.Run{"npm dedupe", []string{}},
+				)
 			}
 
 			return []build.Instruction{
-				{build.Run, []string{"mkdir -p " + tempNpmInstallDir}},
-				{build.Copy, []string{"package.json", tempNpmInstallDir}},
-				{build.Run, []string{"cd " + tempNpmInstallDir + " && " + npmCmd}},
+				build.Run{"mkdir -p", []string{tempNpmInstallDir}},
+				build.Copy{[]string{"package.json"}, tempNpmInstallDir},
+				npmInstall,
 			}
 		case build.PhasePostInstall:
 			return []build.Instruction{
-				{build.Run, []string{"mv " + path.Join(tempNpmInstallDir, "node_modules") + " ./"}},
+				build.Run{"mv", []string{path.Join(tempNpmInstallDir, "node_modules"), "./"}},
 			}
 		}
 	}
diff --git a/config/npm_test.go b/config/npm_test.go
index 69799a9..d73eb75 100644
--- a/config/npm_test.go
+++ b/config/npm_test.go
@@ -65,9 +65,12 @@ func TestNpmConfigInstructionsNonProduction(t *testing.T) {
 	t.Run("PhasePreInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mkdir -p /tmp/node-deps/"}},
-				{build.Copy, []string{"package.json", "/tmp/node-deps/"}},
-				{build.Run, []string{"cd /tmp/node-deps/ && npm install"}},
+				build.Run{"mkdir -p", []string{"/tmp/node-deps/"}},
+				build.Copy{[]string{"package.json"}, "/tmp/node-deps/"},
+				build.RunAll{[]build.Run{
+					{"cd", []string{"/tmp/node-deps/"}},
+					{"npm install", []string{}},
+				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePreInstall),
 		)
@@ -76,7 +79,7 @@ func TestNpmConfigInstructionsNonProduction(t *testing.T) {
 	t.Run("PhasePostInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mv /tmp/node-deps/node_modules ./"}},
+				build.Run{"mv", []string{"/tmp/node-deps/node_modules", "./"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePostInstall),
 		)
@@ -97,9 +100,13 @@ func TestNpmConfigInstructionsProduction(t *testing.T) {
 	t.Run("PhasePreInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mkdir -p /tmp/node-deps/"}},
-				{build.Copy, []string{"package.json", "/tmp/node-deps/"}},
-				{build.Run, []string{"cd /tmp/node-deps/ && npm install --production && npm dedupe"}},
+				build.Run{"mkdir -p", []string{"/tmp/node-deps/"}},
+				build.Copy{[]string{"package.json"}, "/tmp/node-deps/"},
+				build.RunAll{[]build.Run{
+					{"cd", []string{"/tmp/node-deps/"}},
+					{"npm install", []string{"--production"}},
+					{"npm dedupe", []string{}},
+				}},
 			},
 			cfg.InstructionsForPhase(build.PhasePreInstall),
 		)
@@ -108,7 +115,7 @@ func TestNpmConfigInstructionsProduction(t *testing.T) {
 	t.Run("PhasePostInstall", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Run, []string{"mv /tmp/node-deps/node_modules ./"}},
+				build.Run{"mv", []string{"/tmp/node-deps/node_modules", "./"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePostInstall),
 		)
diff --git a/config/runs.go b/config/runs.go
index b2b6623..12184bf 100644
--- a/config/runs.go
+++ b/config/runs.go
@@ -1,8 +1,6 @@
 package config
 
 import (
-	"fmt"
-	"sort"
 	"strconv"
 
 	"phabricator.wikimedia.org/source/blubber.git/build"
@@ -47,53 +45,43 @@ func (run RunsConfig) Home() string {
 	}
 }
 
-func (run RunsConfig) EnvironmentDefinitions() []string {
-	defs := make([]string, 0, len(run.Environment))
-	names := make([]string, 0, len(run.Environment))
-
-	for name := range run.Environment {
-		names = append(names, name)
-	}
-
-	sort.Strings(names)
-
-	for _, name := range names {
-		defs = append(defs, name+"="+strconv.Quote(run.Environment[name]))
-	}
-
-	return defs
-}
-
 func (run RunsConfig) InstructionsForPhase(phase build.Phase) []build.Instruction {
 	ins := []build.Instruction{}
 
 	switch phase {
 	case build.PhasePrivileged:
+		runAll := build.RunAll{}
+
 		if run.In != "" {
-			ins = append(ins, build.Instruction{build.Run, []string{
-				fmt.Sprintf("mkdir -p %q", run.In),
-			}})
+			runAll.Runs = append(runAll.Runs,
+				build.Run{"mkdir -p", []string{run.In}},
+			)
 		}
 
 		if run.As != "" {
-			ins = append(ins, build.Instruction{build.Run, []string{
-				fmt.Sprintf("groupadd -o -g %d -r %q", run.Gid, run.As) + " && " +
-					fmt.Sprintf("useradd -o -m -d %q -r -g %q -u %d %q", run.Home(), run.As, run.Uid, run.As),
-			}})
+			runAll.Runs = append(runAll.Runs,
+				build.Run{"groupadd -o -g %s -r",
+					[]string{strconv.Itoa(run.Gid), run.As}},
+				build.Run{"useradd -o -m -d %s -r -g %s -u %s",
+					[]string{run.Home(), run.As, strconv.Itoa(run.Uid), run.As}},
+			)
 
 			if run.In != "" {
-				ins = append(ins, build.Instruction{build.Run, []string{
-					fmt.Sprintf("chown %q:%q %q", run.As, run.As, run.In),
-				}})
+				runAll.Runs = append(runAll.Runs,
+					build.Run{"chown %s:%s",
+						[]string{run.As, run.As, run.In}},
+				)
 			}
 		}
+
+		if len(runAll.Runs) > 0 {
+			ins = append(ins, runAll)
+		}
 	case build.PhasePrivilegeDropped:
-		ins = append(ins, build.Instruction{build.Env, []string{
-			"HOME=" + strconv.Quote(run.Home()),
-		}})
+		ins = append(ins, build.Env{map[string]string{"HOME": run.Home()}})
 
-		if definitions := run.EnvironmentDefinitions(); len(definitions) > 0 {
-			ins = append(ins, build.Instruction{build.Env, definitions})
+		if len(run.Environment) > 0 {
+			ins = append(ins, build.Env{run.Environment})
 		}
 	}
 
diff --git a/config/runs_test.go b/config/runs_test.go
index cef740c..cb5e1df 100644
--- a/config/runs_test.go
+++ b/config/runs_test.go
@@ -45,22 +45,6 @@ func TestRunsHomeWithoutUser(t *testing.T) {
 	assert.Equal(t, "/root", runs.Home())
 }
 
-func TestEnvironmentDefinitionsIsSortedAndQuoted(t *testing.T) {
-	runs := config.RunsConfig{
-		Environment: map[string]string{
-			"fooname": "foovalue",
-			"barname": "barvalue",
-			"quxname": "quxvalue",
-		},
-	}
-
-	assert.Equal(t, []string{
-		`barname="barvalue"`,
-		`fooname="foovalue"`,
-		`quxname="quxvalue"`,
-	}, runs.EnvironmentDefinitions())
-}
-
 func TestRunsConfigInstructions(t *testing.T) {
 	cfg := config.RunsConfig{
 		As:  "someuser",
@@ -75,14 +59,12 @@ func TestRunsConfigInstructions(t *testing.T) {
 
 	t.Run("PhasePrivileged", func(t *testing.T) {
 		assert.Equal(t,
-			[]build.Instruction{
-				{build.Run, []string{`mkdir -p "/some/directory"`}},
-				{build.Run, []string{
-					`groupadd -o -g 777 -r "someuser" && ` +
-						`useradd -o -m -d "/home/someuser" -r -g "someuser" -u 666 "someuser"`,
-				}},
-				{build.Run, []string{`chown "someuser":"someuser" "/some/directory"`}},
-			},
+			[]build.Instruction{build.RunAll{[]build.Run{
+				{"mkdir -p", []string{"/some/directory"}},
+				{"groupadd -o -g %s -r", []string{"777", "someuser"}},
+				{"useradd -o -m -d %s -r -g %s -u %s", []string{"/home/someuser", "someuser", "666", "someuser"}},
+				{"chown %s:%s", []string{"someuser", "someuser", "/some/directory"}},
+			}}},
 			cfg.InstructionsForPhase(build.PhasePrivileged),
 		)
 	})
@@ -90,8 +72,8 @@ func TestRunsConfigInstructions(t *testing.T) {
 	t.Run("PhasePrivilegeDropped", func(t *testing.T) {
 		assert.Equal(t,
 			[]build.Instruction{
-				{build.Env, []string{`HOME="/home/someuser"`}},
-				{build.Env, []string{`barname="barvalue"`, `fooname="foovalue"`}},
+				build.Env{map[string]string{"HOME": "/home/someuser"}},
+				build.Env{map[string]string{"barname": "barvalue", "fooname": "foovalue"}},
 			},
 			cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
 		)
@@ -101,7 +83,7 @@ func TestRunsConfigInstructions(t *testing.T) {
 
 			assert.Equal(t,
 				[]build.Instruction{
-					{build.Env, []string{`HOME="/home/someuser"`}},
+					build.Env{map[string]string{"HOME": "/home/someuser"}},
 				},
 				cfg.InstructionsForPhase(build.PhasePrivilegeDropped),
 			)