Merge pull request #32495 from dnephin/cleanup-builder-dispatcher-env

Remove Builder.dockerfile field

builder/dockerfile/builder.go

@@ -53,7 +53,6 @@ type Builder struct {
 	clientCtx context.Context
 	cancel    context.CancelFunc
 
-	dockerfile    *parser.Node
 	runConfig     *container.Config // runconfig for cmd, run, entrypoint etc.
 	flags         *BFlags
 	tmpContainers map[string]struct{}
@@ -103,7 +102,7 @@ func (bm *BuildManager) BuildFromContext(ctx context.Context, src io.ReadCloser,
 	if len(dockerfileName) > 0 {
 		buildOptions.Dockerfile = dockerfileName
 	}
-	b, err := NewBuilder(ctx, buildOptions, bm.backend, builder.DockerIgnoreContext{ModifiableContext: buildContext}, nil)
+	b, err := NewBuilder(ctx, buildOptions, bm.backend, builder.DockerIgnoreContext{ModifiableContext: buildContext})
 	if err != nil {
 		return "", err
 	}
@@ -114,7 +113,7 @@ func (bm *BuildManager) BuildFromContext(ctx context.Context, src io.ReadCloser,
 // NewBuilder creates a new Dockerfile builder from an optional dockerfile and a Config.
 // If dockerfile is nil, the Dockerfile specified by Config.DockerfileName,
 // will be read from the Context passed to Build().
-func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, buildContext builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) {
+func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, buildContext builder.Context) (b *Builder, err error) {
 	if config == nil {
 		config = new(types.ImageBuildOptions)
 	}
@@ -139,14 +138,6 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
 	b.imageContexts = &imageContexts{b: b}
 
 	parser.SetEscapeToken(parser.DefaultEscapeToken, &b.directive) // Assume the default token for escape
-
-	if dockerfile != nil {
-		b.dockerfile, err = parser.Parse(dockerfile, &b.directive)
-		if err != nil {
-			return nil, err
-		}
-	}
-
 	return b, nil
 }
 
@@ -193,15 +184,6 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) {
 	return repoAndTags, nil
 }
 
-func (b *Builder) processLabels() {
-	if len(b.options.Labels) == 0 {
-		return
-	}
-
-	node := parser.NodeFromLabels(b.options.Labels)
-	b.dockerfile.Children = append(b.dockerfile.Children, node)
-}
-
 // build runs the Dockerfile builder from a context and a docker object that allows to make calls
 // to Docker.
 //
@@ -222,29 +204,27 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	b.Stderr = stderr
 	b.Output = out
 
-	// If Dockerfile was not parsed yet, extract it from the Context
-	if b.dockerfile == nil {
-		if err := b.readDockerfile(); err != nil {
+	dockerfile, err := b.readDockerfile()
+	if err != nil {
 		return "", err
 	}
-	}
 
 	repoAndTags, err := sanitizeRepoAndTags(b.options.Tags)
 	if err != nil {
 		return "", err
 	}
 
-	b.processLabels()
+	addNodesForLabelOption(dockerfile, b.options.Labels)
 
 	var shortImgID string
-	total := len(b.dockerfile.Children)
-	for _, n := range b.dockerfile.Children {
+	total := len(dockerfile.Children)
+	for _, n := range dockerfile.Children {
 		if err := b.checkDispatch(n, false); err != nil {
 			return "", perrors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
 		}
 	}
 
-	for i, n := range b.dockerfile.Children {
+	for i, n := range dockerfile.Children {
 		select {
 		case <-b.clientCtx.Done():
 			logrus.Debug("Builder: build cancelled!")
@@ -306,6 +286,15 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
 	return b.image, nil
 }
 
+func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) {
+	if len(labels) == 0 {
+		return
+	}
+
+	node := parser.NodeFromLabels(labels)
+	dockerfile.Children = append(dockerfile.Children, node)
+}
+
 // check if there are any leftover build-args that were passed but not
 // consumed during build. Print a warning, if there are any.
 func (b *Builder) warnOnUnusedBuildArgs() {
@@ -335,7 +324,7 @@ func (b *Builder) Cancel() {
 //
 // TODO: Remove?
 func BuildFromConfig(config *container.Config, changes []string) (*container.Config, error) {
-	b, err := NewBuilder(context.Background(), nil, nil, nil, nil)
+	b, err := NewBuilder(context.Background(), nil, nil, nil)
 	if err != nil {
 		return nil, err
 	}

builder/dockerfile/builder_test.go

@@ -2,45 +2,34 @@ package dockerfile
 
 import (
 	"strings"
-
 	"testing"
 
-	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/pkg/testutil/assert"
 )
 
-func TestBuildProcessLabels(t *testing.T) {
+func TestAddNodesForLabelOption(t *testing.T) {
 	dockerfile := "FROM scratch"
 	d := parser.Directive{}
 	parser.SetEscapeToken(parser.DefaultEscapeToken, &d)
-	n, err := parser.Parse(strings.NewReader(dockerfile), &d)
+	nodes, err := parser.Parse(strings.NewReader(dockerfile), &d)
 	assert.NilError(t, err)
 
-	options := &types.ImageBuildOptions{
-		Labels: map[string]string{
+	labels := map[string]string{
 		"org.e": "cli-e",
 		"org.d": "cli-d",
 		"org.c": "cli-c",
 		"org.b": "cli-b",
 		"org.a": "cli-a",
-		},
 	}
-	b := &Builder{
-		runConfig:  &container.Config{},
-		options:    options,
-		directive:  d,
-		dockerfile: n,
-	}
-	b.processLabels()
+	addNodesForLabelOption(nodes, labels)
 
 	expected := []string{
 		"FROM scratch",
 		`LABEL "org.a"='cli-a' "org.b"='cli-b' "org.c"='cli-c' "org.d"='cli-d' "org.e"='cli-e'`,
 	}
-	assert.Equal(t, len(b.dockerfile.Children), 2)
-	for i, v := range b.dockerfile.Children {
+	assert.Equal(t, len(nodes.Children), 2)
+	for i, v := range nodes.Children {
 		assert.Equal(t, v.Original, expected[i])
 	}
 }

builder/dockerfile/internals.go

@@ -650,7 +650,7 @@ func (b *Builder) clearTmp() {
 }
 
 // readDockerfile reads a Dockerfile from the current context.
-func (b *Builder) readDockerfile() error {
+func (b *Builder) readDockerfile() (*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.
@@ -664,10 +664,9 @@ func (b *Builder) readDockerfile() error {
 		}
 	}
 
-	err := b.parseDockerfile()
-
+	nodes, err := b.parseDockerfile()
 	if err != nil {
-		return err
+		return nodes, err
 	}
 
 	// After the Dockerfile has been parsed, we need to check the .dockerignore
@@ -681,32 +680,27 @@ func (b *Builder) readDockerfile() error {
 	if dockerIgnore, ok := b.context.(builder.DockerIgnoreContext); ok {
 		dockerIgnore.Process([]string{b.options.Dockerfile})
 	}
-	return nil
+	return nodes, nil
 }
 
-func (b *Builder) parseDockerfile() error {
+func (b *Builder) parseDockerfile() (*parser.Node, error) {
 	f, err := b.context.Open(b.options.Dockerfile)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
+			return nil, fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
 		}
-		return err
+		return nil, err
 	}
 	defer f.Close()
 	if f, ok := f.(*os.File); ok {
 		// ignoring error because Open already succeeded
 		fi, err := f.Stat()
 		if err != nil {
-			return fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
+			return nil, fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
 		}
 		if fi.Size() == 0 {
-			return fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
+			return nil, fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
 		}
 	}
-	b.dockerfile, err = parser.Parse(f, &b.directive)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return parser.Parse(f, &b.directive)
 }

builder/dockerfile/internals_test.go

@@ -2,12 +2,12 @@ package dockerfile
 
 import (
 	"fmt"
-	"strings"
 	"testing"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/pkg/archive"
+	"github.com/docker/docker/pkg/testutil/assert"
 )
 
 func TestEmptyDockerfile(t *testing.T) {
@@ -54,10 +54,7 @@ func TestNonExistingDockerfile(t *testing.T) {
 
 func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath, expectedError string) {
 	tarStream, err := archive.Tar(contextDir, archive.Uncompressed)
-
-	if err != nil {
-		t.Fatalf("Error when creating tar stream: %s", err)
-	}
+	assert.NilError(t, err)
 
 	defer func() {
 		if err = tarStream.Close(); err != nil {
@@ -66,10 +63,7 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
 	}()
 
 	context, err := builder.MakeTarSumContext(tarStream)
-
-	if err != nil {
-		t.Fatalf("Error when creating tar context: %s", err)
-	}
+	assert.NilError(t, err)
 
 	defer func() {
 		if err = context.Close(); err != nil {
@@ -83,13 +77,6 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
 
 	b := &Builder{options: options, context: context}
 
-	err = b.readDockerfile()
-
-	if err == nil {
-		t.Fatalf("No error when executing test: %s", testName)
-	}
-
-	if !strings.Contains(err.Error(), expectedError) {
-		t.Fatalf("Wrong error message. Should be \"%s\". Got \"%s\"", expectedError, err.Error())
-	}
+	_, err = b.readDockerfile()
+	assert.Error(t, err, expectedError)
 }