From 3dcab289821ddd4575b7e48d463ba8ef2af492ea Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sat, 22 Apr 2017 18:34:04 -0400 Subject: [PATCH] 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 --- builder/dockerfile/dispatchers.go | 7 ------- builder/dockerfile/evaluator.go | 9 ++++++++- builder/dockerfile/internals.go | 10 ++-------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 715161270d..f83a17ce33 100644 --- a/builder/dockerfile/dispatchers.go +++ b/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 { diff --git a/builder/dockerfile/evaluator.go b/builder/dockerfile/evaluator.go index abad15e096..e98a4e2f0d 100644 --- a/builder/dockerfile/evaluator.go +++ b/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) diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index a772c615a8..ec411c4638 100644 --- a/builder/dockerfile/internals.go +++ b/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 }