Simplify Docker instruction compilation

Summary:
After adding a number of new instructions, it seemed there was a lot of
redundant implementation between concrete types. This refactor uses just
one concrete unexported type and a few options for determining the
output format instead of one struct type per possible Docker
instruction.

The exported `docker.Instruction` interface and signature of
`docker.NewInstruction` is unchanged with this refactor. Unit tests for
Docker instruction compilation were simplified to not make type
assertions but were otherwise left to ensure no regression.

Depends on D984

Test Plan: Run `go test ./...`.

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

Reviewed By: thcipriani, #release-engineering-team

Tags: #release-engineering-team

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

2 files changed, 102 insertions, 178 deletions

diff --git a/docker/instructions.go b/docker/instructions.go
index d6e560f..c031d01 100644
--- a/docker/instructions.go
+++ b/docker/instructions.go
@@ -13,157 +13,90 @@ import (
 // instruction is partially compiled at this point by calling Compile() which
 // applies its own logic for escaping arguments, etc.
 //
-func NewInstruction(instruction build.Instruction) (Instruction, error) {
-	switch instruction.(type) {
+func NewInstruction(bi build.Instruction) (Instruction, error) {
+	i := instruction{arguments: bi.Compile()}
+
+	switch bi.(type) {
 	case build.Run, build.RunAll:
-		var dockerInstruction Run
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
-	case build.Copy:
-		var dockerInstruction Copy
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
-	case build.CopyAs:
-		var dockerInstruction CopyAs
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
-	case build.CopyFrom:
-		var dockerInstruction CopyFrom
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
+		i.name = "RUN"
+
+	case build.Copy, build.CopyAs, build.CopyFrom:
+		i.name = "COPY"
+		i.array = true
+
+		switch bi.(type) {
+		case build.CopyAs:
+			i.flags = []string{"chown"}
+		case build.CopyFrom:
+			i.flags = []string{"from"}
+		}
+
 	case build.Env:
-		var dockerInstruction Env
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
+		i.name = "ENV"
+		i.separator = " "
+
 	case build.Label:
-		var dockerInstruction Label
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
+		i.name = "LABEL"
+		i.separator = " "
+
 	case build.User:
-		var dockerInstruction User
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
+		i.name = "USER"
+
 	case build.Volume:
-		var dockerInstruction Volume
-		dockerInstruction.arguments = instruction.Compile()
-		return dockerInstruction, nil
+		i.name = "VOLUME"
+		i.array = true
+	}
+
+	if i.name == "" {
+		return nil, errors.New("Unable to create Instruction")
 	}
 
-	return nil, errors.New("Unable to create Instruction")
+	return i, nil
 }
 
 // Instruction defines an interface for instruction compilation.
 //
 type Instruction interface {
 	Compile() string
-	Arguments() []string
-}
-
-type abstractInstruction struct {
-	arguments []string
-}
-
-func (di abstractInstruction) Arguments() []string {
-	return di.arguments
-}
-
-// Run compiles into a RUN instruction.
-//
-type Run struct{ abstractInstruction }
-
-// Compile compiles RUN instructions.
-//
-func (dr Run) Compile() string {
-	return fmt.Sprintf(
-		"RUN %s\n",
-		join(dr.arguments, ""))
-}
-
-// Copy compiles into a COPY instruction.
-//
-type Copy struct{ abstractInstruction }
-
-// Compile compiles COPY instructions.
-//
-func (dc Copy) Compile() string {
-	return fmt.Sprintf(
-		"COPY [%s]\n",
-		join(dc.arguments, ", "))
 }
 
-// CopyAs compiles into a COPY --chown instruction.
-//
-type CopyAs struct{ abstractInstruction }
-
-// Compile compiles COPY --chown instructions.
-//
-func (dca CopyAs) Compile() string {
-	return fmt.Sprintf(
-		"COPY --chown=%s [%s]\n",
-		dca.arguments[0],
-		join(dca.arguments[1:], ", "))
+type instruction struct {
+	name      string   // name (e.g. "RUN")
+	flags     []string // flags (e.g. "chown")
+	arguments []string // quoted arguments
+	separator string   // argument separator
+	array     bool     // format arguments as array (enforces ", " separator)
 }
 
-// CopyFrom compiles into a COPY --from instruction.
-//
-type CopyFrom struct{ abstractInstruction }
-
-// Compile compiles COPY --from instructions.
-//
-func (dcf CopyFrom) Compile() string {
-	return fmt.Sprintf(
-		"COPY --from=%s [%s]\n",
-		dcf.arguments[0],
-		join(dcf.arguments[1:], ", "))
-}
-
-// Env compiles into a ENV instruction.
-//
-type Env struct{ abstractInstruction }
-
-// Compile compiles ENV instructions.
+// Compile returns a valid Dockerfile line for the instruction.
 //
-func (de Env) Compile() string {
-	return fmt.Sprintf(
-		"ENV %s\n",
-		join(de.arguments, " "))
-}
-
-// Label compiles into a LABEL instruction.
+// Output is in the format "<name> <flags> <arguments>", e.g.
+// "COPY --chown=123:223 ["foo", "bar"]" and flag values are taken from the
+// beginning of the arguments slice.
 //
-type Label struct{ abstractInstruction }
+func (ins instruction) Compile() string {
+	format := ins.name + " "
+	numFlags := len(ins.flags)
+	args := make([]interface{}, numFlags+1)
 
-// Compile returns multiple key="value" arguments as a single LABEL
-// instruction.
-//
-func (dl Label) Compile() string {
-	return fmt.Sprintf(
-		"LABEL %s\n",
-		join(dl.arguments, " "))
-}
+	for i, option := range ins.flags {
+		format += "--" + option + "=%s "
+		args[i] = ins.arguments[i]
+	}
 
-// User compiles into a USER instruction.
-//
-type User struct{ abstractInstruction }
+	separator := ins.separator
 
-// Compile compiles USER instructions.
-//
-func (du User) Compile() string {
-	return fmt.Sprintf(
-		"USER %s\n",
-		join(du.arguments, ", "))
-}
+	if ins.array {
+		separator = ", "
+		format += "[%s]"
+	} else {
+		format += "%s"
+	}
 
-// Volume compiles into a VOLUME instruction.
-//
-type Volume struct{ abstractInstruction }
+	format += "\n"
+	args[numFlags] = join(ins.arguments[numFlags:], separator)
 
-// Compile compiles VOLUME instructions.
-//
-func (dv Volume) Compile() string {
-	return fmt.Sprintf(
-		"VOLUME [%s]\n",
-		join(dv.arguments, ", "))
+	return fmt.Sprintf(format, args...)
 }
 
 func join(arguments []string, delimiter string) string {
diff --git a/docker/instructions_test.go b/docker/instructions_test.go
index 651e0bd..37eeaf1 100644
--- a/docker/instructions_test.go
+++ b/docker/instructions_test.go
@@ -13,11 +13,9 @@ func TestRun(t *testing.T) {
 	i := build.Run{"echo", []string{"hello"}}
 	di, err := docker.NewInstruction(i)
 
-	var dockerRun docker.Run
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerRun, di)
-	assert.Equal(t, "RUN echo \"hello\"\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "RUN echo \"hello\"\n", di.Compile())
+	}
 }
 
 func TestRunAll(t *testing.T) {
@@ -28,11 +26,9 @@ func TestRunAll(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerRun docker.Run
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerRun, di)
-	assert.Equal(t, "RUN echo \"hello\" && echo \"yo\"\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "RUN echo \"hello\" && echo \"yo\"\n", di.Compile())
+	}
 }
 
 func TestCopy(t *testing.T) {
@@ -40,11 +36,9 @@ func TestCopy(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerCopy docker.Copy
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerCopy, di)
-	assert.Equal(t, "COPY [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "COPY [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	}
 }
 
 func TestCopyAs(t *testing.T) {
@@ -52,11 +46,9 @@ func TestCopyAs(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerCopyAs docker.CopyAs
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerCopyAs, di)
-	assert.Equal(t, "COPY --chown=123:124 [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "COPY --chown=123:124 [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	}
 }
 
 func TestCopyFrom(t *testing.T) {
@@ -64,11 +56,9 @@ func TestCopyFrom(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerCopyFrom docker.CopyFrom
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerCopyFrom, di)
-	assert.Equal(t, "COPY --from=foo [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "COPY --from=foo [\"foo1\", \"foo2\", \"bar\"]\n", di.Compile())
+	}
 }
 
 func TestEnv(t *testing.T) {
@@ -76,11 +66,9 @@ func TestEnv(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerEnv docker.Env
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerEnv, di)
-	assert.Equal(t, "ENV bar=\"foo\" foo=\"bar\"\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "ENV bar=\"foo\" foo=\"bar\"\n", di.Compile())
+	}
 }
 
 func TestLabel(t *testing.T) {
@@ -88,11 +76,9 @@ func TestLabel(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerLabel docker.Label
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerLabel, di)
-	assert.Equal(t, "LABEL bar=\"foo\" foo=\"bar\"\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "LABEL bar=\"foo\" foo=\"bar\"\n", di.Compile())
+	}
 }
 
 func TestUser(t *testing.T) {
@@ -100,11 +86,9 @@ func TestUser(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerUser docker.User
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerUser, di)
-	assert.Equal(t, "USER \"foo\"\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "USER \"foo\"\n", di.Compile())
+	}
 }
 
 func TestVolume(t *testing.T) {
@@ -112,30 +96,37 @@ func TestVolume(t *testing.T) {
 
 	di, err := docker.NewInstruction(i)
 
-	var dockerVolume docker.Volume
-
-	assert.Nil(t, err)
-	assert.IsType(t, dockerVolume, di)
-	assert.Equal(t, "VOLUME [\"/foo/dir\"]\n", di.Compile())
+	if assert.NoError(t, err) {
+		assert.Equal(t, "VOLUME [\"/foo/dir\"]\n", di.Compile())
+	}
 }
 
 func TestEscapeRun(t *testing.T) {
 	i := build.Run{"/bin/true\nRUN echo HACKED!", []string{}}
-	dr, _ := docker.NewInstruction(i)
 
-	assert.Equal(t, "RUN /bin/true\\nRUN echo HACKED!\n", dr.Compile())
+	di, err := docker.NewInstruction(i)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, "RUN /bin/true\\nRUN echo HACKED!\n", di.Compile())
+	}
 }
 
 func TestEscapeCopy(t *testing.T) {
 	i := build.Copy{[]string{"file.a", "file.b"}, "dest"}
-	dr, _ := docker.NewInstruction(i)
 
-	assert.Equal(t, "COPY [\"file.a\", \"file.b\", \"dest\"]\n", dr.Compile())
+	di, err := docker.NewInstruction(i)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, "COPY [\"file.a\", \"file.b\", \"dest\"]\n", di.Compile())
+	}
 }
 
 func TestEscapeEnv(t *testing.T) {
 	i := build.Env{map[string]string{"a": "b\nRUN echo HACKED!"}}
-	dr, _ := docker.NewInstruction(i)
 
-	assert.Equal(t, "ENV a=\"b\\nRUN echo HACKED!\"\n", dr.Compile())
+	di, err := docker.NewInstruction(i)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, "ENV a=\"b\\nRUN echo HACKED!\"\n", di.Compile())
+	}
 }