Fix setting b.runConfig.Image at arbitrary places.

Previously this value was set at some point attrbitrarily between when it was updated and when it was going to be used next.

Instead always set it as the last step of dispatch.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
Upstream-commit: 3dcab289821ddd4575b7e48d463ba8ef2af492ea
Component: engine

components/engine/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,9 +368,6 @@ func run(req dispatchRequest) error {
 		saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs)
 	}
 
-	// TODO: this was previously in b.create(), why is it necessary?
-	req.runConfig.Image = req.builder.image
-
 	runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd))
 	hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe)
 	if err != nil || hit {

components/engine/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)

components/engine/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)
@@ -100,10 +98,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
@@ -542,12 +536,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
 	}