Factor out functions from builder/dockerfile/builder.go:Builder.build()

Remove the block comment which is stale, and redundant now that the
function is just as readable as the comment.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

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
+	if err := checkDispatchDockerfile(dockerfile); err != nil {
-	total := len(dockerfile.Children)
+		return "", err
-	for _, n := range dockerfile.Children {
+	}
-		if err := b.checkDispatch(n, false); err != nil {
+
-			return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
+	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?")
 }
 
-	if b.options.Squash {
+func (b *Builder) squashBuild() error {
 	var fromID string
+	var err error
 	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")
+		return errors.Wrap(err, "error squashing image")
 	}
-	}
+	return nil
-
-	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
 }
 
 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 <image>` 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)
+	if err := checkDispatchDockerfile(ast); err != nil {
-	for _, n := range ast.Children {
-		if err := b.checkDispatch(n, false); err != nil {
 		return nil, err
 	}
-	}
 
-	for i, n := range ast.Children {
+	if err := dispatchFromDockerfile(b, ast); err != nil {
-		if err := b.dispatch(i, total, n); 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
+}

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 checkDispatch(ast *parser.Node) error {
-func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) 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
 	}

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
-			}
-		}
-		for i, n := range ast.Children {
-			if err := b.dispatch(i, total, 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)
 			}
 		}
 
+		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.
+// readAndParseDockerfile reads a Dockerfile from the current context.
-func (b *Builder) readDockerfile() (*parser.Node, error) {
+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.

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)
 }