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

2 files changed, 57 insertions, 21 deletions

diff --git a/docker/instructions.go b/docker/instructions.go
index 39400b3..a93ad11 100644
--- a/docker/instructions.go
+++ b/docker/instructions.go
@@ -3,25 +3,24 @@ package docker
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
 
 	"phabricator.wikimedia.org/source/blubber.git/build"
 )
 
 func NewDockerInstruction(instruction build.Instruction) (DockerInstruction, error) {
-	switch instruction.Type {
-	case build.Run:
+	switch instruction.(type) {
+	case build.Run, build.RunAll:
 		var dockerInstruction DockerRun
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	case build.Copy:
 		var dockerInstruction DockerCopy
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	case build.Env:
 		var dockerInstruction DockerEnv
-		dockerInstruction.arguments = instruction.Arguments
+		dockerInstruction.arguments = instruction.Compile()
 		return dockerInstruction, nil
 	}
 	return nil, errors.New("Unable to create DockerInstruction")
@@ -52,9 +51,8 @@ type DockerCopy struct{ abstractDockerInstruction }
 
 func (dc DockerCopy) Compile() string {
 	return fmt.Sprintf(
-		"COPY [%s, %s]\n",
-		removeNewlines(strconv.Quote(dc.arguments[0])),
-		removeNewlines(strconv.Quote(dc.arguments[1])))
+		"COPY [%s]\n",
+		removeNewlines(strings.Join(dc.arguments, ", ")))
 }
 
 type DockerEnv struct{ abstractDockerInstruction }
diff --git a/docker/instructions_test.go b/docker/instructions_test.go
index 66dac61..c03d48b 100644
--- a/docker/instructions_test.go
+++ b/docker/instructions_test.go
@@ -8,35 +8,73 @@ import (
 	"phabricator.wikimedia.org/source/blubber.git/docker"
 )
 
-func TestFactory(t *testing.T) {
-	i := build.Instruction{build.Run, []string{"echo hello"}}
-	dr, _ := docker.NewDockerInstruction(i)
+func TestRun(t *testing.T) {
+	i := build.Run{"echo", []string{"hello"}}
+	di, err := docker.NewDockerInstruction(i)
 
 	var dockerRun docker.DockerRun
 
-	assert.IsType(t, dockerRun, dr)
-	assert.Equal(t, dr.Arguments(), i.Arguments)
-	assert.NotEmpty(t, dr.Compile())
-	assert.Equal(t, "RUN echo hello\n", dr.Compile())
+	assert.Nil(t, err)
+	assert.IsType(t, dockerRun, di)
+	assert.Equal(t, "RUN echo \"hello\"\n", di.Compile())
+}
+
+func TestRunAll(t *testing.T) {
+	i := build.RunAll{[]build.Run{
+		{"echo", []string{"hello"}},
+		{"echo", []string{"yo"}},
+	}}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerRun docker.DockerRun
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerRun, di)
+	assert.Equal(t, "RUN echo \"hello\" && echo \"yo\"\n", di.Compile())
+}
+
+func TestCopy(t *testing.T) {
+	i := build.Copy{[]string{"foo1", "foo2"}, "bar"}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerCopy docker.DockerCopy
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerCopy, di)
+	assert.Equal(t, "COPY [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+}
+
+func TestEnv(t *testing.T) {
+	i := build.Env{map[string]string{"foo": "bar", "bar": "foo"}}
+
+	di, err := docker.NewDockerInstruction(i)
+
+	var dockerEnv docker.DockerEnv
+
+	assert.Nil(t, err)
+	assert.IsType(t, dockerEnv, di)
+	assert.Equal(t, "ENV bar=\"foo\" foo=\"bar\"\n", di.Compile())
 }
 
 func TestEscapeRun(t *testing.T) {
-	i := build.Instruction{build.Run, []string{"/bin/true\nRUN echo HACKED!"}}
+	i := build.Run{"/bin/true\nRUN echo HACKED!", []string{}}
 	dr, _ := docker.NewDockerInstruction(i)
 
 	assert.Equal(t, "RUN /bin/true\\nRUN echo HACKED!\n", dr.Compile())
 }
 
 func TestEscapeCopy(t *testing.T) {
-	i := build.Instruction{build.Copy, []string{"file.a", "file.b"}}
+	i := build.Copy{[]string{"file.a", "file.b"}, "dest"}
 	dr, _ := docker.NewDockerInstruction(i)
 
-	assert.Equal(t, "COPY [\"file.a\", \"file.b\"]\n", dr.Compile())
+	assert.Equal(t, "COPY [\"file.a\", \"file.b\", \"dest\"]\n", dr.Compile())
 }
 
 func TestEscapeEnv(t *testing.T) {
-	i := build.Instruction{build.Env, []string{"a=b\nRUN echo HACKED!"}}
+	i := build.Env{map[string]string{"a": "b\nRUN echo HACKED!"}}
 	dr, _ := docker.NewDockerInstruction(i)
 
-	assert.Equal(t, "ENV a=b\\nRUN echo HACKED!\n", dr.Compile())
+	assert.Equal(t, "ENV a=\"b\\nRUN echo HACKED!\"\n", dr.Compile())
 }