Browse code

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>

Daniel Nephin authored on 2017/04/23 07:34:04
Showing 3 changed files
... ...
@@ -315,10 +315,6 @@ func workdir(req dispatchRequest) error {
315 315
 		return nil
316 316
 	}
317 317
 
318
-	// TODO: why is this done here. This seems to be done at random places all over
319
-	// the builder
320
-	req.runConfig.Image = req.builder.image
321
-
322 318
 	comment := "WORKDIR " + req.runConfig.WorkingDir
323 319
 	runConfigWithCommentCmd := copyRunConfig(req.runConfig, withCmdCommentString(comment))
324 320
 	if hit, err := req.builder.probeCache(req.builder.image, runConfigWithCommentCmd); err != nil || hit {
... ...
@@ -372,9 +368,6 @@ func run(req dispatchRequest) error {
372 372
 		saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs)
373 373
 	}
374 374
 
375
-	// TODO: this was previously in b.create(), why is it necessary?
376
-	req.runConfig.Image = req.builder.image
377
-
378 375
 	runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd))
379 376
 	hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe)
380 377
 	if err != nil || hit {
... ...
@@ -173,7 +173,14 @@ func (b *Builder) dispatch(stepN int, stepTotal int, node *parser.Node, shlex *S
173 173
 	// XXX yes, we skip any cmds that are not valid; the parser should have
174 174
 	// picked these out already.
175 175
 	if f, ok := evaluateTable[cmd]; ok {
176
-		return f(newDispatchRequestFromNode(node, b, strList, shlex))
176
+		if err := f(newDispatchRequestFromNode(node, b, strList, shlex)); err != nil {
177
+			return err
178
+		}
179
+		// TODO: return an object instead of setting things on builder
180
+		// If the step created a new image set it as the imageID for the
181
+		// current runConfig
182
+		b.runConfig.Image = b.image
183
+		return nil
177 184
 	}
178 185
 
179 186
 	return fmt.Errorf("Unknown instruction: %s", upperCasedCmd)
... ...
@@ -42,8 +42,6 @@ func (b *Builder) commit(comment string) error {
42 42
 	if !b.hasFromImage() {
43 43
 		return errors.New("Please provide a source image with `from` prior to commit")
44 44
 	}
45
-	// TODO: why is this set here?
46
-	b.runConfig.Image = b.image
47 45
 
48 46
 	runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment))
49 47
 	hit, err := b.probeCache(b.image, runConfigWithCommentCmd)
... ...
@@ -100,10 +98,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD
100 100
 	// Work in daemon-specific filepath semantics
101 101
 	dest := filepath.FromSlash(args[len(args)-1]) // last one is always the dest
102 102
 
103
-	// TODO: why is this done here. This seems to be done at random places all over
104
-	// the builder
105
-	b.runConfig.Image = b.image
106
-
107 103
 	var infos []copyInfo
108 104
 
109 105
 	// Loop through each src file and calculate the info we need to
... ...
@@ -542,12 +536,12 @@ func (b *Builder) processImageFrom(img builder.Image) error {
542 542
 // If an image is found, probeCache returns `(true, nil)`.
543 543
 // If no image is found, it returns `(false, nil)`.
544 544
 // If there is any error, it returns `(false, err)`.
545
-func (b *Builder) probeCache(imageID string, runConfig *container.Config) (bool, error) {
545
+func (b *Builder) probeCache(parentID string, runConfig *container.Config) (bool, error) {
546 546
 	c := b.imageCache
547 547
 	if c == nil || b.options.NoCache || b.cacheBusted {
548 548
 		return false, nil
549 549
 	}
550
-	cache, err := c.GetCache(imageID, runConfig)
550
+	cache, err := c.GetCache(parentID, runConfig)
551 551
 	if err != nil {
552 552
 		return false, err
553 553
 	}