diff --git a/components/engine/builder/dockerfile/builder.go b/components/engine/builder/dockerfile/builder.go index 03abfafcf3..d655fff7fb 100644 --- a/components/engine/builder/dockerfile/builder.go +++ b/components/engine/builder/dockerfile/builder.go @@ -185,17 +185,6 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) { // build runs the Dockerfile builder from a context and a docker object that allows to make calls // to Docker. -// -// This will (barring errors): -// -// * read the dockerfile from context -// * parse the dockerfile if not already parsed -// * walk the AST and execute it by dispatching to handlers. If Remove -// or ForceRemove is set, additional cleanup around containers happens after -// processing. -// * Tag image, if applicable. -// * Print a happy message and return the image ID. -// func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (string, error) { defer b.imageContexts.unmount() @@ -203,7 +192,7 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri b.Stderr = stderr b.Output = out - dockerfile, err := b.readDockerfile() + dockerfile, err := b.readAndParseDockerfile() if err != nil { return "", err } @@ -215,14 +204,37 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri addNodesForLabelOption(dockerfile, b.options.Labels) - var shortImgID string - total := len(dockerfile.Children) - for _, n := range dockerfile.Children { - if err := b.checkDispatch(n, false); err != nil { - return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine) + if err := checkDispatchDockerfile(dockerfile); err != nil { + return "", err + } + + shortImageID, err := b.dispatchDockerfileWithCancellation(dockerfile) + if err != nil { + return "", err + } + + b.warnOnUnusedBuildArgs() + + if b.image == "" { + return "", errors.New("No image was generated. Is your Dockerfile empty?") + } + + if b.options.Squash { + if err := b.squashBuild(); err != nil { + return "", err } } + fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImageID) + if err := b.tagImages(repoAndTags); err != nil { + return "", err + } + return b.image, nil +} + +func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Node) (string, error) { + total := len(dockerfile.Children) + var shortImgID string for i, n := range dockerfile.Children { select { case <-b.clientCtx.Done(): @@ -255,34 +267,20 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri return "", errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target) } - b.warnOnUnusedBuildArgs() + return shortImgID, nil +} - if b.image == "" { - return "", errors.New("No image was generated. Is your Dockerfile empty?") +func (b *Builder) squashBuild() error { + var fromID string + var err error + if b.from != nil { + fromID = b.from.ImageID() } - - if b.options.Squash { - var fromID string - if b.from != nil { - fromID = b.from.ImageID() - } - b.image, err = b.docker.SquashImage(b.image, fromID) - if err != nil { - return "", errors.Wrap(err, "error squashing image") - } + b.image, err = b.docker.SquashImage(b.image, fromID) + if err != nil { + return errors.Wrap(err, "error squashing image") } - - fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImgID) - - imageID := image.ID(b.image) - for _, rt := range repoAndTags { - if err := b.docker.TagImageWithReference(imageID, rt); err != nil { - return "", err - } - fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt)) - } - - return b.image, nil + return nil } func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) { @@ -303,6 +301,17 @@ func (b *Builder) warnOnUnusedBuildArgs() { } } +func (b *Builder) tagImages(repoAndTags []reference.Named) error { + imageID := image.ID(b.image) + for _, rt := range repoAndTags { + if err := b.docker.TagImageWithReference(imageID, rt); err != nil { + return err + } + fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt)) + } + return nil +} + // hasFromImage returns true if the builder has processed a `FROM ` line func (b *Builder) hasFromImage() bool { return b.image != "" || b.noBaseImage @@ -345,18 +354,31 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con b.Stderr = ioutil.Discard b.disableCommit = true - total := len(ast.Children) - for _, n := range ast.Children { - if err := b.checkDispatch(n, false); err != nil { - return nil, err - } + if err := checkDispatchDockerfile(ast); err != nil { + return nil, err } - for i, n := range ast.Children { - if err := b.dispatch(i, total, n); err != nil { - return nil, err - } + if err := dispatchFromDockerfile(b, ast); err != nil { + return nil, err } - return b.runConfig, nil } + +func checkDispatchDockerfile(dockerfile *parser.Node) error { + for _, n := range dockerfile.Children { + if err := checkDispatch(n); err != nil { + return errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine) + } + } + return nil +} + +func dispatchFromDockerfile(b *Builder, ast *parser.Node) error { + total := len(ast.Children) + for i, n := range ast.Children { + if err := b.dispatch(i, total, n); err != nil { + return err + } + } + return nil +} diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go index 5be23c8cda..c456ba41f7 100644 --- a/components/engine/builder/dockerfile/evaluator.go +++ b/components/engine/builder/dockerfile/evaluator.go @@ -206,8 +206,7 @@ func (b *Builder) runConfigEnvMapping() map[string]string { // arg, env, etc., this syntax check will not be complete and could not replace // the runtime check. Instead, this function is only a helper that allows // user to find out the obvious error in Dockerfile earlier on. -// onbuild bool: indicate if instruction XXX is part of `ONBUILD XXX` trigger -func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error { +func checkDispatch(ast *parser.Node) error { cmd := ast.Value upperCasedCmd := strings.ToUpper(cmd) @@ -225,16 +224,6 @@ func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error { } } - // The instruction is part of ONBUILD trigger (not the instruction itself) - if onbuild { - switch upperCasedCmd { - case "ONBUILD": - return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") - case "MAINTAINER", "FROM": - return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd) - } - } - if _, ok := evaluateTable[cmd]; ok { return nil } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 122bb2019e..32698775d1 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -467,19 +467,24 @@ func (b *Builder) processImageFrom(img builder.Image) error { return err } - total := len(ast.Children) for _, n := range ast.Children { - if err := b.checkDispatch(n, true); err != nil { + if err := checkDispatch(n); err != nil { return err } + + upperCasedCmd := strings.ToUpper(n.Value) + switch upperCasedCmd { + case "ONBUILD": + return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed") + case "MAINTAINER", "FROM": + return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd) + } } - for i, n := range ast.Children { - if err := b.dispatch(i, total, n); err != nil { - return err - } + + if err := dispatchFromDockerfile(b, ast); err != nil { + return err } } - return nil } @@ -644,8 +649,8 @@ func (b *Builder) clearTmp() { } } -// readDockerfile reads a Dockerfile from the current context. -func (b *Builder) readDockerfile() (*parser.Node, error) { +// readAndParseDockerfile reads a Dockerfile from the current context. +func (b *Builder) readAndParseDockerfile() (*parser.Node, error) { // If no -f was specified then look for 'Dockerfile'. If we can't find // that then look for 'dockerfile'. If neither are found then default // back to 'Dockerfile' and use that in the error message. diff --git a/components/engine/builder/dockerfile/internals_test.go b/components/engine/builder/dockerfile/internals_test.go index 56d5e4f182..1bffecfa75 100644 --- a/components/engine/builder/dockerfile/internals_test.go +++ b/components/engine/builder/dockerfile/internals_test.go @@ -77,6 +77,6 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath, b := &Builder{options: options, context: context} - _, err = b.readDockerfile() + _, err = b.readAndParseDockerfile() assert.Error(t, err, expectedError) }