From f3e205ddc15040ec80ef047959afa7b75586a491 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 4 Apr 2017 14:44:40 -0400 Subject: [PATCH] Remove the dockerfile field from Builder. Return dockerfile from parseDockerfile and pass the dockerfile nodes as an arg Strip unused arg from builder.NewBuilder. Signed-off-by: Daniel Nephin --- builder/dockerfile/builder.go | 49 +++++++++++----------------- builder/dockerfile/builder_test.go | 33 +++++++------------ builder/dockerfile/internals.go | 26 ++++++--------- builder/dockerfile/internals_test.go | 23 +++---------- 4 files changed, 45 insertions(+), 86 deletions(-) diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index c4f7d20440..ba1c8f9981 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -52,7 +52,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{} @@ -102,7 +101,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 } @@ -113,7 +112,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) } @@ -138,14 +137,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 } @@ -192,15 +183,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. // @@ -221,11 +203,9 @@ 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 { - return "", err - } + dockerfile, err := b.readDockerfile() + if err != nil { + return "", err } repoAndTags, err := sanitizeRepoAndTags(b.options.Tags) @@ -233,17 +213,17 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri 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!") @@ -297,6 +277,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() { @@ -326,7 +315,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 } diff --git a/builder/dockerfile/builder_test.go b/builder/dockerfile/builder_test.go index c9e5453aee..9a5a17ef4d 100644 --- a/builder/dockerfile/builder_test.go +++ b/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{ - "org.e": "cli-e", - "org.d": "cli-d", - "org.c": "cli-c", - "org.b": "cli-b", - "org.a": "cli-a", - }, + 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]) } } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 6b2461f1dc..eecd47fdea 100644 --- a/builder/dockerfile/internals.go +++ b/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) } diff --git a/builder/dockerfile/internals_test.go b/builder/dockerfile/internals_test.go index a250c5e0ca..56d5e4f182 100644 --- a/builder/dockerfile/internals_test.go +++ b/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) }