Merge pull request #32749 from dnephin/rm-ContainerUpdateCmdOnBuild

[Builder] Remove ContainerUpdateCmdOnBuild

builder/builder.go

@@ -54,8 +54,6 @@ type Backend interface {
 	ContainerStart(containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error
 	// ContainerWait stops processing until the given container is stopped.
 	ContainerWait(containerID string, timeout time.Duration) (int, error)
-	// ContainerUpdateCmdOnBuild updates container.Path and container.Args
-	ContainerUpdateCmdOnBuild(containerID string, cmd []string) error
 	// ContainerCreateWorkdir creates the workdir
 	ContainerCreateWorkdir(containerID string) error
 

builder/dockerfile/dispatchers.go

@@ -315,10 +315,6 @@ func workdir(req dispatchRequest) error {
 		return nil
 	}
 
-	// TODO: why is this done here. This seems to be done at random places all over
-	// the builder
-	req.runConfig.Image = req.builder.image
-
 	comment := "WORKDIR " + req.runConfig.WorkingDir
 	runConfigWithCommentCmd := copyRunConfig(req.runConfig, withCmdCommentString(comment))
 	if hit, err := req.builder.probeCache(req.builder.image, runConfigWithCommentCmd); err != nil || hit {
@@ -372,10 +368,9 @@ func run(req dispatchRequest) error {
 		saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs)
 	}
 
-	// TODO: this was previously in b.create(), why is it necessary?
+	runConfigForCacheProbe := copyRunConfig(req.runConfig,
-	req.runConfig.Image = req.builder.image
+		withCmd(saveCmd),
-
+		withEntrypointOverride(saveCmd, nil))
-	runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd))
 	hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe)
 	if err != nil || hit {
 		return err
@@ -383,13 +378,13 @@ func run(req dispatchRequest) error {
 
 	runConfig := copyRunConfig(req.runConfig,
 		withCmd(cmdFromArgs),
-		withEnv(append(req.runConfig.Env, buildArgs...)))
+		withEnv(append(req.runConfig.Env, buildArgs...)),
+		withEntrypointOverride(saveCmd, strslice.StrSlice{""}))
 
 	// set config as already being escaped, this prevents double escaping on windows
 	runConfig.ArgsEscaped = true
 
 	logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd)
-
 	cID, err := req.builder.create(runConfig)
 	if err != nil {
 		return err

builder/dockerfile/dispatchers_test.go

@@ -448,6 +448,7 @@ func TestRunWithBuildArgs(t *testing.T) {
 		getCacheFunc: func(parentID string, cfg *container.Config) (string, error) {
 			// Check the runConfig.Cmd sent to probeCache()
 			assert.Equal(t, cachedCmd, cfg.Cmd)
+			assert.Equal(t, strslice.StrSlice(nil), cfg.Entrypoint)
 			return "", nil
 		},
 	}
@@ -462,12 +463,14 @@ func TestRunWithBuildArgs(t *testing.T) {
 		// Check the runConfig.Cmd sent to create()
 		assert.Equal(t, cmdWithShell, config.Config.Cmd)
 		assert.Contains(t, config.Config.Env, "one=two")
+		assert.Equal(t, strslice.StrSlice{""}, config.Config.Entrypoint)
 		return container.ContainerCreateCreatedBody{ID: "12345"}, nil
 	}
 	mockBackend.commitFunc = func(cID string, cfg *backend.ContainerCommitConfig) (string, error) {
 		// Check the runConfig.Cmd sent to commit()
 		assert.Equal(t, origCmd, cfg.Config.Cmd)
 		assert.Equal(t, cachedCmd, cfg.ContainerConfig.Cmd)
+		assert.Equal(t, strslice.StrSlice(nil), cfg.Config.Entrypoint)
 		return "", nil
 	}
 

builder/dockerfile/evaluator.go

@@ -173,7 +173,14 @@ func (b *Builder) dispatch(stepN int, stepTotal int, node *parser.Node, shlex *S
 	// XXX yes, we skip any cmds that are not valid; the parser should have
 	// picked these out already.
 	if f, ok := evaluateTable[cmd]; ok {
-		return f(newDispatchRequestFromNode(node, b, strList, shlex))
+		if err := f(newDispatchRequestFromNode(node, b, strList, shlex)); err != nil {
+			return err
+		}
+		// TODO: return an object instead of setting things on builder
+		// If the step created a new image set it as the imageID for the
+		// current runConfig
+		b.runConfig.Image = b.image
+		return nil
 	}
 
 	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)

builder/dockerfile/internals.go

@@ -42,8 +42,6 @@ func (b *Builder) commit(comment string) error {
 	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
 
 	runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment))
 	hit, err := b.probeCache(b.image, runConfigWithCommentCmd)
@@ -67,7 +65,8 @@ func (b *Builder) commitContainer(id string, containerConfig *container.Config)
 		ContainerCommitConfig: types.ContainerCommitConfig{
 			Author: b.maintainer,
 			Pause:  true,
-			Config: b.runConfig,
+			// TODO: this should be done by Commit()
+			Config: copyRunConfig(b.runConfig),
 		},
 		ContainerConfig: containerConfig,
 	}
@@ -100,10 +99,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
 	// Work in daemon-specific filepath semantics
 	dest := filepath.FromSlash(args[len(args)-1]) // last one is always the dest
 
-	// TODO: why is this done here. This seems to be done at random places all over
-	// the builder
-	b.runConfig.Image = b.image
-
 	var infos []copyInfo
 
 	// Loop through each src file and calculate the info we need to
@@ -239,6 +234,21 @@ func withEnv(env []string) runConfigModifier {
 	}
 }
 
+// withEntrypointOverride sets an entrypoint on runConfig if the command is
+// not empty. The entrypoint is left unmodified if command is empty.
+//
+// The dockerfile RUN instruction expect to run without an entrypoint
+// so the runConfig entrypoint needs to be modified accordingly. ContainerCreate
+// will change a []string{""} entrypoint to nil, so we probe the cache with the
+// nil entrypoint.
+func withEntrypointOverride(cmd []string, entrypoint []string) runConfigModifier {
+	return func(runConfig *container.Config) {
+		if len(cmd) > 0 {
+			runConfig.Entrypoint = entrypoint
+		}
+	}
+}
+
 // 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 {
@@ -541,12 +551,12 @@ 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(imageID string, runConfig *container.Config) (bool, error) {
+func (b *Builder) probeCache(parentID string, runConfig *container.Config) (bool, error) {
 	c := b.imageCache
 	if c == nil || b.options.NoCache || b.cacheBusted {
 		return false, nil
 	}
-	cache, err := c.GetCache(imageID, runConfig)
+	cache, err := c.GetCache(parentID, runConfig)
 	if err != nil {
 		return false, err
 	}
@@ -606,12 +616,6 @@ func (b *Builder) create(runConfig *container.Config) (string, error) {
 
 	b.tmpContainers[c.ID] = struct{}{}
 	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, runConfig.Cmd); err != nil {
-		return "", err
-	}
-
 	return c.ID, nil
 }
 

builder/dockerfile/mockbackend_test.go

@@ -73,10 +73,6 @@ func (m *MockBackend) ContainerWait(containerID string, timeout time.Duration) (
 	return 0, nil
 }
 
-func (m *MockBackend) ContainerUpdateCmdOnBuild(containerID string, cmd []string) error {
-	return nil
-}
-
 func (m *MockBackend) ContainerCreateWorkdir(containerID string) error {
 	return nil
 }

daemon/update.go

@@ -22,20 +22,6 @@ func (daemon *Daemon) ContainerUpdate(name string, hostConfig *container.HostCon
 	return container.ContainerUpdateOKBody{Warnings: warnings}, nil
 }
 
-// ContainerUpdateCmdOnBuild updates Path and Args for the container with ID cID.
-func (daemon *Daemon) ContainerUpdateCmdOnBuild(cID string, cmd []string) error {
-	if len(cmd) == 0 {
-		return nil
-	}
-	c, err := daemon.GetContainer(cID)
-	if err != nil {
-		return err
-	}
-	c.Path = cmd[0]
-	c.Args = cmd[1:]
-	return nil
-}
-
 func (daemon *Daemon) update(name string, hostConfig *container.HostConfig) error {
 	if hostConfig == nil {
 		return nil

integration-cli/docker_cli_build_test.go

@@ -337,13 +337,13 @@ func (s *DockerSuite) TestBuildOnBuildCmdEntrypointJSON(c *check.C) {
 	name1 := "onbuildcmd"
 	name2 := "onbuildgenerated"
 
-	buildImageSuccessfully(c, name1, build.WithDockerfile(`
+	cli.BuildCmd(c, name1, build.WithDockerfile(`
 FROM busybox
 ONBUILD CMD ["hello world"]
 ONBUILD ENTRYPOINT ["echo"]
 ONBUILD RUN ["true"]`))
 
-	buildImageSuccessfully(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1)))
+	cli.BuildCmd(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1)))
 
 	result := cli.DockerCmd(c, "run", name2)
 	result.Assert(c, icmd.Expected{Out: "hello world"})
@@ -1785,11 +1785,17 @@ func (s *DockerSuite) TestBuildConditionalCache(c *check.C) {
 	}
 }
 
-// FIXME(vdemeester) this really seems to test the same thing as before
 func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check.C) {
 	name := "testbuildaddmultiplelocalfilewithcache"
-	dockerfile := `
+	baseName := name + "-base"
+
+	cli.BuildCmd(c, baseName, build.WithDockerfile(`
 		FROM busybox
+		ENTRYPOINT ["/bin/sh"]
+	`))
+
+	dockerfile := `
+		FROM testbuildaddmultiplelocalfilewithcache-base
         MAINTAINER dockerio
         ADD foo Dockerfile /usr/lib/bla/
 		RUN sh -c "[ $(cat /usr/lib/bla/foo) = "hello" ]"`
@@ -1799,15 +1805,15 @@ func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check.
 	defer ctx.Close()
 	cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx))
 	id1 := getIDByName(c, name)
-	cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx))
+	result2 := cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx))
 	id2 := getIDByName(c, name)
-	cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx))
+	result3 := cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx))
 	id3 := getIDByName(c, name)
 	if id1 != id2 {
-		c.Fatal("The cache should have been used but hasn't.")
+		c.Fatalf("The cache should have been used but hasn't: %s", result2.Stdout())
 	}
 	if id1 == id3 {
-		c.Fatal("The cache should have been invalided but hasn't.")
+		c.Fatalf("The cache should have been invalided but hasn't: %s", result3.Stdout())
 	}
 }