Extract commitContainer from commit() to separate the distinct branches

Remove unused arguments to commit.
This will allow us to remove all the runConfig mutate+revert code that
is scattered around builder dispatchers/internals

Signed-off-by: Daniel Nephin <dnephin@docker.com>

builder/dockerfile/dispatchers.go

@@ -73,7 +73,7 @@ func env(req dispatchRequest) error {
 		}
 	}
 
-	return req.builder.commit("", req.runConfig.Cmd, commitMessage.String())
+	return req.builder.commit(commitMessage.String())
 }
 
 // MAINTAINER some text <maybe@an.email.address>
@@ -90,7 +90,7 @@ func maintainer(req dispatchRequest) error {
 
 	maintainer := req.args[0]
 	req.builder.maintainer = maintainer
-	return req.builder.commit("", req.runConfig.Cmd, "MAINTAINER "+maintainer)
+	return req.builder.commit("MAINTAINER " + maintainer)
 }
 
 // LABEL some json data describing the image
@@ -130,7 +130,7 @@ func label(req dispatchRequest) error {
 		req.runConfig.Labels[req.args[j]] = req.args[j+1]
 		j++
 	}
-	return req.builder.commit("", req.runConfig.Cmd, commitStr)
+	return req.builder.commit(commitStr)
 }
 
 // ADD foo /path
@@ -281,7 +281,7 @@ func onbuild(req dispatchRequest) error {
 
 	original := regexp.MustCompile(`(?i)^\s*ONBUILD\s*`).ReplaceAllString(req.original, "")
 	req.runConfig.OnBuild = append(req.runConfig.OnBuild, original)
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ONBUILD %s", original))
+	return req.builder.commit("ONBUILD " + original)
 }
 
 // WORKDIR /tmp
@@ -321,10 +321,9 @@ func workdir(req dispatchRequest) error {
 	req.runConfig.Cmd = strslice.StrSlice(append(getShell(req.runConfig), "#(nop) "+comment))
 	defer func(cmd strslice.StrSlice) { req.runConfig.Cmd = cmd }(cmd)
 
-	if hit, err := req.builder.probeCache(); err != nil {
+	// TODO: this should pass a copy of runConfig
+	if hit, err := req.builder.probeCache(req.builder.image, req.runConfig); err != nil || hit {
 		return err
-	} else if hit {
-		return nil
 	}
 
 	req.runConfig.Image = req.builder.image
@@ -341,7 +340,7 @@ func workdir(req dispatchRequest) error {
 		return err
 	}
 
-	return req.builder.commit(container.ID, cmd, comment)
+	return req.builder.commitContainer(container.ID, copyRunConfig(req.runConfig, withCmd(cmd)))
 }
 
 // RUN some command yo
@@ -402,13 +401,10 @@ func run(req dispatchRequest) error {
 	}
 
 	req.runConfig.Cmd = saveCmd
-	hit, err := req.builder.probeCache()
-	if err != nil {
+	hit, err := req.builder.probeCache(req.builder.image, req.runConfig)
+	if err != nil || hit {
 		return err
 	}
-	if hit {
-		return nil
-	}
 
 	// set Cmd manually, this is special case only for Dockerfiles
 	req.runConfig.Cmd = config.Cmd
@@ -419,7 +415,11 @@ func run(req dispatchRequest) error {
 
 	logrus.Debugf("[BUILDER] Command to be executed: %v", req.runConfig.Cmd)
 
-	cID, err := req.builder.create()
+	// TODO: this was previously in b.create(), why is it necessary?
+	req.builder.runConfig.Image = req.builder.image
+
+	// TODO: should pass a copy of runConfig
+	cID, err := req.builder.create(req.runConfig)
 	if err != nil {
 		return err
 	}
@@ -451,7 +451,7 @@ func run(req dispatchRequest) error {
 		saveCmd = strslice.StrSlice(append(tmpEnv, saveCmd...))
 	}
 	req.runConfig.Cmd = saveCmd
-	return req.builder.commit(cID, cmd, "run")
+	return req.builder.commitContainer(cID, copyRunConfig(req.runConfig, withCmd(cmd)))
 }
 
 // CMD foo
@@ -474,7 +474,7 @@ func cmd(req dispatchRequest) error {
 	// set config as already being escaped, this prevents double escaping on windows
 	req.runConfig.ArgsEscaped = true
 
-	if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("CMD %q", cmdSlice)); err != nil {
+	if err := req.builder.commit(fmt.Sprintf("CMD %q", cmdSlice)); err != nil {
 		return err
 	}
 
@@ -590,7 +590,7 @@ func healthcheck(req dispatchRequest) error {
 		req.runConfig.Healthcheck = &healthcheck
 	}
 
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("HEALTHCHECK %q", req.runConfig.Healthcheck))
+	return req.builder.commit(fmt.Sprintf("HEALTHCHECK %q", req.runConfig.Healthcheck))
 }
 
 // ENTRYPOINT /usr/sbin/nginx
@@ -626,11 +626,7 @@ func entrypoint(req dispatchRequest) error {
 		req.runConfig.Cmd = nil
 	}
 
-	if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ENTRYPOINT %q", req.runConfig.Entrypoint)); err != nil {
-		return err
-	}
-
-	return nil
+	return req.builder.commit(fmt.Sprintf("ENTRYPOINT %q", req.runConfig.Entrypoint))
 }
 
 // EXPOSE 6667/tcp 7000/tcp
@@ -671,7 +667,7 @@ func expose(req dispatchRequest) error {
 		i++
 	}
 	sort.Strings(portList)
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("EXPOSE %s", strings.Join(portList, " ")))
+	return req.builder.commit("EXPOSE " + strings.Join(portList, " "))
 }
 
 // USER foo
@@ -689,7 +685,7 @@ func user(req dispatchRequest) error {
 	}
 
 	req.runConfig.User = req.args[0]
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("USER %v", req.args))
+	return req.builder.commit(fmt.Sprintf("USER %v", req.args))
 }
 
 // VOLUME /foo
@@ -715,10 +711,7 @@ func volume(req dispatchRequest) error {
 		}
 		req.runConfig.Volumes[v] = struct{}{}
 	}
-	if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("VOLUME %v", req.args)); err != nil {
-		return err
-	}
-	return nil
+	return req.builder.commit(fmt.Sprintf("VOLUME %v", req.args))
 }
 
 // STOPSIGNAL signal
@@ -736,7 +729,7 @@ func stopSignal(req dispatchRequest) error {
 	}
 
 	req.runConfig.StopSignal = sig
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("STOPSIGNAL %v", req.args))
+	return req.builder.commit(fmt.Sprintf("STOPSIGNAL %v", req.args))
 }
 
 // ARG name[=value]
@@ -786,7 +779,7 @@ func arg(req dispatchRequest) error {
 		req.builder.buildArgs.AddMetaArg(name, value)
 		return nil
 	}
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ARG %s", arg))
+	return req.builder.commit("ARG " + arg)
 }
 
 // SHELL powershell -command
@@ -808,7 +801,7 @@ func shell(req dispatchRequest) error {
 		// SHELL powershell -command - not JSON
 		return errNotJSON("SHELL", req.original)
 	}
-	return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("SHELL %v", shellSlice))
+	return req.builder.commit(fmt.Sprintf("SHELL %v", shellSlice))
 }
 
 func errAtLeastOneArgument(command string) error {
@@ -831,15 +824,6 @@ func errTooManyArguments(command string) error {
 	return fmt.Errorf("Bad input to %s, too many arguments", command)
 }
 
-// getShell is a helper function which gets the right shell for prefixing the
-// shell-form of RUN, ENTRYPOINT and CMD instructions
-func getShell(c *container.Config) []string {
-	if 0 == len(c.Shell) {
-		return append([]string{}, defaultShell[:]...)
-	}
-	return append([]string{}, c.Shell[:]...)
-}
-
 // mountByRef creates an imageMount from a reference. pulling the image if needed.
 func mountByRef(b *Builder, name string) (*imageMount, error) {
 	image, err := pullOrGetImage(b, name)

builder/dockerfile/internals.go

@@ -36,41 +36,39 @@ import (
 	"github.com/pkg/errors"
 )
 
-func (b *Builder) commit(id string, autoCmd strslice.StrSlice, comment string) error {
+func (b *Builder) commit(comment string) error {
 	if b.disableCommit {
 		return nil
 	}
 	if !b.hasFromImage() {
 		return errors.New("Please provide a source image with `from` prior to commit")
 	}
+	// TODO: why is this set here?
 	b.runConfig.Image = b.image
 
-	if id == "" {
-		cmd := b.runConfig.Cmd
-		b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), "#(nop) ", comment))
-		defer func(cmd strslice.StrSlice) { b.runConfig.Cmd = cmd }(cmd)
-
-		hit, err := b.probeCache()
-		if err != nil {
-			return err
-		} else if hit {
-			return nil
-		}
-		id, err = b.create()
-		if err != nil {
-			return err
-		}
+	runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment))
+	hit, err := b.probeCache(b.image, runConfigWithCommentCmd)
+	if err != nil || hit {
+		return err
+	}
+	id, err := b.create(runConfigWithCommentCmd)
+	if err != nil {
+		return err
 	}
 
-	// Note: Actually copy the struct
-	autoConfig := *b.runConfig
-	autoConfig.Cmd = autoCmd
+	return b.commitContainer(id, b.runConfig)
+}
+
+func (b *Builder) commitContainer(id string, runConfig *container.Config) error {
+	if b.disableCommit {
+		return nil
+	}
 
 	commitCfg := &backend.ContainerCommitConfig{
 		ContainerCommitConfig: types.ContainerCommitConfig{
 			Author: b.maintainer,
 			Pause:  true,
-			Config: &autoConfig,
+			Config: runConfig,
 		},
 	}
 
@@ -80,8 +78,10 @@ func (b *Builder) commit(id string, autoCmd strslice.StrSlice, comment string) e
 		return err
 	}
 
+	// TODO: this function should return imageID and runConfig instead of setting
+	// then on the builder
 	b.image = imageID
-	b.imageContexts.update(imageID, &autoConfig)
+	b.imageContexts.update(imageID, runConfig)
 	return nil
 }
 
@@ -148,11 +148,9 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 	// For backwards compat, if there's just one info then use it as the
 	// cache look-up string, otherwise hash 'em all into one
 	var srcHash string
-	var origPaths string
 
 	if len(infos) == 1 {
 		info := infos[0]
-		origPaths = info.path
 		srcHash = info.hash
 	} else {
 		var hashs []string
@@ -164,17 +162,16 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 		hasher := sha256.New()
 		hasher.Write([]byte(strings.Join(hashs, ",")))
 		srcHash = "multi:" + hex.EncodeToString(hasher.Sum(nil))
-		origPaths = strings.Join(origs, " ")
 	}
 
 	cmd := b.runConfig.Cmd
+	// TODO: should this have been using origPaths instead of srcHash in the comment?
 	b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), fmt.Sprintf("#(nop) %s %s in %s ", cmdName, srcHash, dest)))
 	defer func(cmd strslice.StrSlice) { b.runConfig.Cmd = cmd }(cmd)
 
-	if hit, err := b.probeCache(); err != nil {
+	// TODO: this should pass a copy of runConfig
+	if hit, err := b.probeCache(b.image, b.runConfig); err != nil || hit {
 		return err
-	} else if hit {
-		return nil
 	}
 
 	container, err := b.docker.ContainerCreate(types.ContainerCreateConfig{
@@ -187,8 +184,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 	}
 	b.tmpContainers[container.ID] = struct{}{}
 
-	comment := fmt.Sprintf("%s %s in %s", cmdName, origPaths, dest)
-
 	// Twiddle the destination when it's a relative path - meaning, make it
 	// relative to the WORKINGDIR
 	if dest, err = normaliseDest(cmdName, b.runConfig.WorkingDir, dest); err != nil {
@@ -201,7 +196,44 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 		}
 	}
 
-	return b.commit(container.ID, cmd, comment)
+	return b.commitContainer(container.ID, copyRunConfig(b.runConfig, withCmd(cmd)))
+}
+
+type runConfigModifier func(*container.Config)
+
+func copyRunConfig(runConfig *container.Config, modifiers ...runConfigModifier) *container.Config {
+	copy := *runConfig
+	for _, modifier := range modifiers {
+		modifier(&copy)
+	}
+	return &copy
+}
+
+func withCmd(cmd []string) runConfigModifier {
+	return func(runConfig *container.Config) {
+		runConfig.Cmd = cmd
+	}
+}
+
+func withCmdComment(comment string) runConfigModifier {
+	return func(runConfig *container.Config) {
+		runConfig.Cmd = append(getShell(runConfig), "#(nop) ", comment)
+	}
+}
+
+func withEnv(env []string) runConfigModifier {
+	return func(runConfig *container.Config) {
+		runConfig.Env = env
+	}
+}
+
+// getShell is a helper function which gets the right shell for prefixing the
+// shell-form of RUN, ENTRYPOINT and CMD instructions
+func getShell(c *container.Config) []string {
+	if 0 == len(c.Shell) {
+		return append([]string{}, defaultShell[:]...)
+	}
+	return append([]string{}, c.Shell[:]...)
 }
 
 func (b *Builder) download(srcURL string) (remote builder.Source, p string, err error) {
@@ -498,35 +530,33 @@ func (b *Builder) processImageFrom(img builder.Image) error {
 // If an image is found, probeCache returns `(true, nil)`.
 // If no image is found, it returns `(false, nil)`.
 // If there is any error, it returns `(false, err)`.
-func (b *Builder) probeCache() (bool, error) {
+func (b *Builder) probeCache(imageID string, runConfig *container.Config) (bool, error) {
 	c := b.imageCache
 	if c == nil || b.options.NoCache || b.cacheBusted {
 		return false, nil
 	}
-	cache, err := c.GetCache(b.image, b.runConfig)
+	cache, err := c.GetCache(imageID, runConfig)
 	if err != nil {
 		return false, err
 	}
 	if len(cache) == 0 {
-		logrus.Debugf("[BUILDER] Cache miss: %s", b.runConfig.Cmd)
+		logrus.Debugf("[BUILDER] Cache miss: %s", runConfig.Cmd)
 		b.cacheBusted = true
 		return false, nil
 	}
 
 	fmt.Fprint(b.Stdout, " ---> Using cache\n")
-	logrus.Debugf("[BUILDER] Use cached version: %s", b.runConfig.Cmd)
+	logrus.Debugf("[BUILDER] Use cached version: %s", runConfig.Cmd)
 	b.image = string(cache)
-	b.imageContexts.update(b.image, b.runConfig)
+	b.imageContexts.update(b.image, runConfig)
 
 	return true, nil
 }
 
-func (b *Builder) create() (string, error) {
+func (b *Builder) create(runConfig *container.Config) (string, error) {
 	if !b.hasFromImage() {
 		return "", errors.New("Please provide a source image with `from` prior to run")
 	}
-	b.runConfig.Image = b.image
-
 	resources := container.Resources{
 		CgroupParent: b.options.CgroupParent,
 		CPUShares:    b.options.CPUShares,
@@ -551,11 +581,9 @@ func (b *Builder) create() (string, error) {
 		ExtraHosts: b.options.ExtraHosts,
 	}
 
-	config := *b.runConfig
-
 	// Create the container
 	c, err := b.docker.ContainerCreate(types.ContainerCreateConfig{
-		Config:     b.runConfig,
+		Config:     runConfig,
 		HostConfig: hostConfig,
 	})
 	if err != nil {
@@ -569,7 +597,7 @@ func (b *Builder) create() (string, error) {
 	fmt.Fprintf(b.Stdout, " ---> Running in %s\n", stringid.TruncateID(c.ID))
 
 	// override the entry point that may have been picked up from the base image
-	if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, config.Cmd); err != nil {
+	if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, runConfig.Cmd); err != nil {
 		return "", err
 	}
 

builder/dockerfile/internals_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/archive"
@@ -71,3 +72,54 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
 	_, _, err = remotecontext.Detect(context.Background(), "", dockerfilePath, tarStream, nil)
 	assert.EqualError(t, err, expectedError)
 }
+
+func TestCopyRunConfig(t *testing.T) {
+	defaultEnv := []string{"foo=1"}
+	defaultCmd := []string{"old"}
+
+	var testcases = []struct {
+		doc       string
+		modifiers []runConfigModifier
+		expected  *container.Config
+	}{
+		{
+			doc:       "Set the command",
+			modifiers: []runConfigModifier{withCmd([]string{"new"})},
+			expected: &container.Config{
+				Cmd: []string{"new"},
+				Env: defaultEnv,
+			},
+		},
+		{
+			doc:       "Set the command to a comment",
+			modifiers: []runConfigModifier{withCmdComment("comment")},
+			expected: &container.Config{
+				Cmd: append(defaultShell, "#(nop) ", "comment"),
+				Env: defaultEnv,
+			},
+		},
+		{
+			doc: "Set the command and env",
+			modifiers: []runConfigModifier{
+				withCmd([]string{"new"}),
+				withEnv([]string{"one", "two"}),
+			},
+			expected: &container.Config{
+				Cmd: []string{"new"},
+				Env: []string{"one", "two"},
+			},
+		},
+	}
+
+	for _, testcase := range testcases {
+		runConfig := &container.Config{
+			Cmd: defaultCmd,
+			Env: defaultEnv,
+		}
+		runConfigCopy := copyRunConfig(runConfig, testcase.modifiers...)
+		assert.Equal(t, testcase.expected, runConfigCopy, testcase.doc)
+		// Assert the original was not modified
+		assert.NotEqual(t, runConfig, runConfigCopy, testcase.doc)
+	}
+
+}