Browse code

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 <dnephin@docker.com>

Daniel Nephin authored on 2017/04/05 03:44:40
Showing 4 changed files
... ...
@@ -52,7 +52,6 @@ type Builder struct {
52 52
 	clientCtx context.Context
53 53
 	cancel    context.CancelFunc
54 54
 
55
-	dockerfile    *parser.Node
56 55
 	runConfig     *container.Config // runconfig for cmd, run, entrypoint etc.
57 56
 	flags         *BFlags
58 57
 	tmpContainers map[string]struct{}
... ...
@@ -102,7 +101,7 @@ func (bm *BuildManager) BuildFromContext(ctx context.Context, src io.ReadCloser,
102 102
 	if len(dockerfileName) > 0 {
103 103
 		buildOptions.Dockerfile = dockerfileName
104 104
 	}
105
-	b, err := NewBuilder(ctx, buildOptions, bm.backend, builder.DockerIgnoreContext{ModifiableContext: buildContext}, nil)
105
+	b, err := NewBuilder(ctx, buildOptions, bm.backend, builder.DockerIgnoreContext{ModifiableContext: buildContext})
106 106
 	if err != nil {
107 107
 		return "", err
108 108
 	}
... ...
@@ -113,7 +112,7 @@ func (bm *BuildManager) BuildFromContext(ctx context.Context, src io.ReadCloser,
113 113
 // NewBuilder creates a new Dockerfile builder from an optional dockerfile and a Config.
114 114
 // If dockerfile is nil, the Dockerfile specified by Config.DockerfileName,
115 115
 // will be read from the Context passed to Build().
116
-func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, buildContext builder.Context, dockerfile io.ReadCloser) (b *Builder, err error) {
116
+func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, backend builder.Backend, buildContext builder.Context) (b *Builder, err error) {
117 117
 	if config == nil {
118 118
 		config = new(types.ImageBuildOptions)
119 119
 	}
... ...
@@ -138,14 +137,6 @@ func NewBuilder(clientCtx context.Context, config *types.ImageBuildOptions, back
138 138
 	b.imageContexts = &imageContexts{b: b}
139 139
 
140 140
 	parser.SetEscapeToken(parser.DefaultEscapeToken, &b.directive) // Assume the default token for escape
141
-
142
-	if dockerfile != nil {
143
-		b.dockerfile, err = parser.Parse(dockerfile, &b.directive)
144
-		if err != nil {
145
-			return nil, err
146
-		}
147
-	}
148
-
149 141
 	return b, nil
150 142
 }
151 143
 
... ...
@@ -192,15 +183,6 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) {
192 192
 	return repoAndTags, nil
193 193
 }
194 194
 
195
-func (b *Builder) processLabels() {
196
-	if len(b.options.Labels) == 0 {
197
-		return
198
-	}
199
-
200
-	node := parser.NodeFromLabels(b.options.Labels)
201
-	b.dockerfile.Children = append(b.dockerfile.Children, node)
202
-}
203
-
204 195
 // build runs the Dockerfile builder from a context and a docker object that allows to make calls
205 196
 // to Docker.
206 197
 //
... ...
@@ -221,11 +203,9 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
221 221
 	b.Stderr = stderr
222 222
 	b.Output = out
223 223
 
224
-	// If Dockerfile was not parsed yet, extract it from the Context
225
-	if b.dockerfile == nil {
226
-		if err := b.readDockerfile(); err != nil {
227
-			return "", err
228
-		}
224
+	dockerfile, err := b.readDockerfile()
225
+	if err != nil {
226
+		return "", err
229 227
 	}
230 228
 
231 229
 	repoAndTags, err := sanitizeRepoAndTags(b.options.Tags)
... ...
@@ -233,17 +213,17 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
233 233
 		return "", err
234 234
 	}
235 235
 
236
-	b.processLabels()
236
+	addNodesForLabelOption(dockerfile, b.options.Labels)
237 237
 
238 238
 	var shortImgID string
239
-	total := len(b.dockerfile.Children)
240
-	for _, n := range b.dockerfile.Children {
239
+	total := len(dockerfile.Children)
240
+	for _, n := range dockerfile.Children {
241 241
 		if err := b.checkDispatch(n, false); err != nil {
242 242
 			return "", perrors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
243 243
 		}
244 244
 	}
245 245
 
246
-	for i, n := range b.dockerfile.Children {
246
+	for i, n := range dockerfile.Children {
247 247
 		select {
248 248
 		case <-b.clientCtx.Done():
249 249
 			logrus.Debug("Builder: build cancelled!")
... ...
@@ -297,6 +277,15 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
297 297
 	return b.image, nil
298 298
 }
299 299
 
300
+func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) {
301
+	if len(labels) == 0 {
302
+		return
303
+	}
304
+
305
+	node := parser.NodeFromLabels(labels)
306
+	dockerfile.Children = append(dockerfile.Children, node)
307
+}
308
+
300 309
 // check if there are any leftover build-args that were passed but not
301 310
 // consumed during build. Print a warning, if there are any.
302 311
 func (b *Builder) warnOnUnusedBuildArgs() {
... ...
@@ -326,7 +315,7 @@ func (b *Builder) Cancel() {
326 326
 //
327 327
 // TODO: Remove?
328 328
 func BuildFromConfig(config *container.Config, changes []string) (*container.Config, error) {
329
-	b, err := NewBuilder(context.Background(), nil, nil, nil, nil)
329
+	b, err := NewBuilder(context.Background(), nil, nil, nil)
330 330
 	if err != nil {
331 331
 		return nil, err
332 332
 	}
... ...
@@ -2,45 +2,34 @@ package dockerfile
2 2
 
3 3
 import (
4 4
 	"strings"
5
-
6 5
 	"testing"
7 6
 
8
-	"github.com/docker/docker/api/types"
9
-	"github.com/docker/docker/api/types/container"
10 7
 	"github.com/docker/docker/builder/dockerfile/parser"
11 8
 	"github.com/docker/docker/pkg/testutil/assert"
12 9
 )
13 10
 
14
-func TestBuildProcessLabels(t *testing.T) {
11
+func TestAddNodesForLabelOption(t *testing.T) {
15 12
 	dockerfile := "FROM scratch"
16 13
 	d := parser.Directive{}
17 14
 	parser.SetEscapeToken(parser.DefaultEscapeToken, &d)
18
-	n, err := parser.Parse(strings.NewReader(dockerfile), &d)
15
+	nodes, err := parser.Parse(strings.NewReader(dockerfile), &d)
19 16
 	assert.NilError(t, err)
20 17
 
21
-	options := &types.ImageBuildOptions{
22
-		Labels: map[string]string{
23
-			"org.e": "cli-e",
24
-			"org.d": "cli-d",
25
-			"org.c": "cli-c",
26
-			"org.b": "cli-b",
27
-			"org.a": "cli-a",
28
-		},
29
-	}
30
-	b := &Builder{
31
-		runConfig:  &container.Config{},
32
-		options:    options,
33
-		directive:  d,
34
-		dockerfile: n,
18
+	labels := map[string]string{
19
+		"org.e": "cli-e",
20
+		"org.d": "cli-d",
21
+		"org.c": "cli-c",
22
+		"org.b": "cli-b",
23
+		"org.a": "cli-a",
35 24
 	}
36
-	b.processLabels()
25
+	addNodesForLabelOption(nodes, labels)
37 26
 
38 27
 	expected := []string{
39 28
 		"FROM scratch",
40 29
 		`LABEL "org.a"='cli-a' "org.b"='cli-b' "org.c"='cli-c' "org.d"='cli-d' "org.e"='cli-e'`,
41 30
 	}
42
-	assert.Equal(t, len(b.dockerfile.Children), 2)
43
-	for i, v := range b.dockerfile.Children {
31
+	assert.Equal(t, len(nodes.Children), 2)
32
+	for i, v := range nodes.Children {
44 33
 		assert.Equal(t, v.Original, expected[i])
45 34
 	}
46 35
 }
... ...
@@ -650,7 +650,7 @@ func (b *Builder) clearTmp() {
650 650
 }
651 651
 
652 652
 // readDockerfile reads a Dockerfile from the current context.
653
-func (b *Builder) readDockerfile() error {
653
+func (b *Builder) readDockerfile() (*parser.Node, error) {
654 654
 	// If no -f was specified then look for 'Dockerfile'. If we can't find
655 655
 	// that then look for 'dockerfile'.  If neither are found then default
656 656
 	// back to 'Dockerfile' and use that in the error message.
... ...
@@ -664,10 +664,9 @@ func (b *Builder) readDockerfile() error {
664 664
 		}
665 665
 	}
666 666
 
667
-	err := b.parseDockerfile()
668
-
667
+	nodes, err := b.parseDockerfile()
669 668
 	if err != nil {
670
-		return err
669
+		return nodes, err
671 670
 	}
672 671
 
673 672
 	// After the Dockerfile has been parsed, we need to check the .dockerignore
... ...
@@ -681,32 +680,27 @@ func (b *Builder) readDockerfile() error {
681 681
 	if dockerIgnore, ok := b.context.(builder.DockerIgnoreContext); ok {
682 682
 		dockerIgnore.Process([]string{b.options.Dockerfile})
683 683
 	}
684
-	return nil
684
+	return nodes, nil
685 685
 }
686 686
 
687
-func (b *Builder) parseDockerfile() error {
687
+func (b *Builder) parseDockerfile() (*parser.Node, error) {
688 688
 	f, err := b.context.Open(b.options.Dockerfile)
689 689
 	if err != nil {
690 690
 		if os.IsNotExist(err) {
691
-			return fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
691
+			return nil, fmt.Errorf("Cannot locate specified Dockerfile: %s", b.options.Dockerfile)
692 692
 		}
693
-		return err
693
+		return nil, err
694 694
 	}
695 695
 	defer f.Close()
696 696
 	if f, ok := f.(*os.File); ok {
697 697
 		// ignoring error because Open already succeeded
698 698
 		fi, err := f.Stat()
699 699
 		if err != nil {
700
-			return fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
700
+			return nil, fmt.Errorf("Unexpected error reading Dockerfile: %v", err)
701 701
 		}
702 702
 		if fi.Size() == 0 {
703
-			return fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
703
+			return nil, fmt.Errorf("The Dockerfile (%s) cannot be empty", b.options.Dockerfile)
704 704
 		}
705 705
 	}
706
-	b.dockerfile, err = parser.Parse(f, &b.directive)
707
-	if err != nil {
708
-		return err
709
-	}
710
-
711
-	return nil
706
+	return parser.Parse(f, &b.directive)
712 707
 }
... ...
@@ -2,12 +2,12 @@ package dockerfile
2 2
 
3 3
 import (
4 4
 	"fmt"
5
-	"strings"
6 5
 	"testing"
7 6
 
8 7
 	"github.com/docker/docker/api/types"
9 8
 	"github.com/docker/docker/builder"
10 9
 	"github.com/docker/docker/pkg/archive"
10
+	"github.com/docker/docker/pkg/testutil/assert"
11 11
 )
12 12
 
13 13
 func TestEmptyDockerfile(t *testing.T) {
... ...
@@ -54,10 +54,7 @@ func TestNonExistingDockerfile(t *testing.T) {
54 54
 
55 55
 func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath, expectedError string) {
56 56
 	tarStream, err := archive.Tar(contextDir, archive.Uncompressed)
57
-
58
-	if err != nil {
59
-		t.Fatalf("Error when creating tar stream: %s", err)
60
-	}
57
+	assert.NilError(t, err)
61 58
 
62 59
 	defer func() {
63 60
 		if err = tarStream.Close(); err != nil {
... ...
@@ -66,10 +63,7 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
66 66
 	}()
67 67
 
68 68
 	context, err := builder.MakeTarSumContext(tarStream)
69
-
70
-	if err != nil {
71
-		t.Fatalf("Error when creating tar context: %s", err)
72
-	}
69
+	assert.NilError(t, err)
73 70
 
74 71
 	defer func() {
75 72
 		if err = context.Close(); err != nil {
... ...
@@ -83,13 +77,6 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
83 83
 
84 84
 	b := &Builder{options: options, context: context}
85 85
 
86
-	err = b.readDockerfile()
87
-
88
-	if err == nil {
89
-		t.Fatalf("No error when executing test: %s", testName)
90
-	}
91
-
92
-	if !strings.Contains(err.Error(), expectedError) {
93
-		t.Fatalf("Wrong error message. Should be \"%s\". Got \"%s\"", expectedError, err.Error())
94
-	}
86
+	_, err = b.readDockerfile()
87
+	assert.Error(t, err, expectedError)
95 88
 }