Skip to content

Commit bfcd958

Browse files
dnephindnephin
committed
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>
1 parent c7fad9b commit bfcd958

File tree

4 files changed

+87
-71
lines changed

4 files changed

+87
-71
lines changed

‎builder/dockerfile/builder.go‎

Lines changed: 71 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -185,25 +185,14 @@ func sanitizeRepoAndTags(names []string) ([]reference.Named, error) {
185185

186186
// build runs the Dockerfile builder from a context and a docker object that allows to make calls
187187
// to Docker.
188-
//
189-
// This will (barring errors):
190-
//
191-
// * read the dockerfile from context
192-
// * parse the dockerfile if not already parsed
193-
// * walk the AST and execute it by dispatching to handlers. If Remove
194-
// or ForceRemove is set, additional cleanup around containers happens after
195-
// processing.
196-
// * Tag image, if applicable.
197-
// * Print a happy message and return the image ID.
198-
//
199188
func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (string, error) {
200189
defer b.imageContexts.unmount()
201190

202191
b.Stdout = stdout
203192
b.Stderr = stderr
204193
b.Output = out
205194

206-
dockerfile, err := b.readDockerfile()
195+
dockerfile, err := b.readAndParseDockerfile()
207196
if err != nil {
208197
return "", err
209198
}
@@ -215,14 +204,37 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
215204

216205
addNodesForLabelOption(dockerfile, b.options.Labels)
217206

218-
var shortImgID string
219-
total := len(dockerfile.Children)
220-
for _, n := range dockerfile.Children {
221-
if err := b.checkDispatch(n, false); err != nil {
222-
return "", errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
207+
if err := checkDispatchDockerfile(dockerfile); err != nil {
208+
return "", err
209+
}
210+
211+
shortImageID, err := b.dispatchDockerfileWithCancellation(dockerfile)
212+
if err != nil {
213+
return "", err
214+
}
215+
216+
b.warnOnUnusedBuildArgs()
217+
218+
if b.image == "" {
219+
return "", errors.New("No image was generated. Is your Dockerfile empty?")
220+
}
221+
222+
if b.options.Squash {
223+
if err := b.squashBuild(); err != nil {
224+
return "", err
223225
}
224226
}
225227

228+
fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImageID)
229+
if err := b.tagImages(repoAndTags); err != nil {
230+
return "", err
231+
}
232+
return b.image, nil
233+
}
234+
235+
func (b *Builder) dispatchDockerfileWithCancellation(dockerfile *parser.Node) (string, error) {
236+
total := len(dockerfile.Children)
237+
var shortImgID string
226238
for i, n := range dockerfile.Children {
227239
select {
228240
case <-b.clientCtx.Done():
@@ -255,34 +267,20 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
255267
return "", errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target)
256268
}
257269

258-
b.warnOnUnusedBuildArgs()
259-
260-
if b.image == "" {
261-
return "", errors.New("No image was generated. Is your Dockerfile empty?")
262-
}
270+
return shortImgID, nil
271+
}
263272

264-
if b.options.Squash {
265-
var fromID string
266-
if b.from != nil {
267-
fromID = b.from.ImageID()
268-
}
269-
b.image, err = b.docker.SquashImage(b.image, fromID)
270-
if err != nil {
271-
return "", errors.Wrap(err, "error squashing image")
272-
}
273+
func (b *Builder) squashBuild() error {
274+
var fromID string
275+
var err error
276+
if b.from != nil {
277+
fromID = b.from.ImageID()
273278
}
274-
275-
fmt.Fprintf(b.Stdout, "Successfully built %s\n", shortImgID)
276-
277-
imageID := image.ID(b.image)
278-
for _, rt := range repoAndTags {
279-
if err := b.docker.TagImageWithReference(imageID, rt); err != nil {
280-
return "", err
281-
}
282-
fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt))
279+
b.image, err = b.docker.SquashImage(b.image, fromID)
280+
if err != nil {
281+
return errors.Wrap(err, "error squashing image")
283282
}
284-
285-
return b.image, nil
283+
return nil
286284
}
287285

288286
func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) {
@@ -303,6 +301,17 @@ func (b *Builder) warnOnUnusedBuildArgs() {
303301
}
304302
}
305303

304+
func (b *Builder) tagImages(repoAndTags []reference.Named) error {
305+
imageID := image.ID(b.image)
306+
for _, rt := range repoAndTags {
307+
if err := b.docker.TagImageWithReference(imageID, rt); err != nil {
308+
return err
309+
}
310+
fmt.Fprintf(b.Stdout, "Successfully tagged %s\n", reference.FamiliarString(rt))
311+
}
312+
return nil
313+
}
314+
306315
// hasFromImage returns true if the builder has processed a `FROM <image>` line
307316
func (b *Builder) hasFromImage() bool {
308317
return b.image != "" || b.noBaseImage
@@ -345,18 +354,31 @@ func BuildFromConfig(config *container.Config, changes []string) (*container.Con
345354
b.Stderr = ioutil.Discard
346355
b.disableCommit = true
347356

348-
total := len(ast.Children)
349-
for _, n := range ast.Children {
350-
if err := b.checkDispatch(n, false); err != nil {
351-
return nil, err
357+
if err := checkDispatchDockerfile(ast); err != nil {
358+
return nil, err
359+
}
360+
361+
if err := dispatchFromDockerfile(b, ast); err != nil {
362+
return nil, err
363+
}
364+
return b.runConfig, nil
365+
}
366+
367+
func checkDispatchDockerfile(dockerfile *parser.Node) error {
368+
for _, n := range dockerfile.Children {
369+
if err := checkDispatch(n); err != nil {
370+
return errors.Wrapf(err, "Dockerfile parse error line %d", n.StartLine)
352371
}
353372
}
373+
return nil
374+
}
354375

376+
func dispatchFromDockerfile(b *Builder, ast *parser.Node) error {
377+
total := len(ast.Children)
355378
for i, n := range ast.Children {
356379
if err := b.dispatch(i, total, n); err != nil {
357-
return nil, err
380+
return err
358381
}
359382
}
360-
361-
return b.runConfig, nil
383+
return nil
362384
}

‎builder/dockerfile/evaluator.go‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,7 @@ func (b *Builder) runConfigEnvMapping() map[string]string {
206206
// arg, env, etc., this syntax check will not be complete and could not replace
207207
// the runtime check. Instead, this function is only a helper that allows
208208
// user to find out the obvious error in Dockerfile earlier on.
209-
// onbuild bool: indicate if instruction XXX is part of `ONBUILD XXX` trigger
210-
func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
209+
func checkDispatch(ast *parser.Node) error {
211210
cmd := ast.Value
212211
upperCasedCmd := strings.ToUpper(cmd)
213212

@@ -225,16 +224,6 @@ func (b *Builder) checkDispatch(ast *parser.Node, onbuild bool) error {
225224
}
226225
}
227226

228-
// The instruction is part of ONBUILD trigger (not the instruction itself)
229-
if onbuild {
230-
switch upperCasedCmd {
231-
case "ONBUILD":
232-
return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
233-
case "MAINTAINER", "FROM":
234-
return fmt.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
235-
}
236-
}
237-
238227
if _, ok := evaluateTable[cmd]; ok {
239228
return nil
240229
}

‎builder/dockerfile/internals.go‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -467,19 +467,24 @@ func (b *Builder) processImageFrom(img builder.Image) error {
467467
return err
468468
}
469469

470-
total := len(ast.Children)
471470
for _, n := range ast.Children {
472-
if err := b.checkDispatch(n, true); err != nil {
471+
if err := checkDispatch(n); err != nil {
473472
return err
474473
}
475-
}
476-
for i, n := range ast.Children {
477-
if err := b.dispatch(i, total, n); err != nil {
478-
return err
474+
475+
upperCasedCmd := strings.ToUpper(n.Value)
476+
switch upperCasedCmd {
477+
case "ONBUILD":
478+
return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
479+
case "MAINTAINER", "FROM":
480+
return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
479481
}
480482
}
481-
}
482483

484+
if err := dispatchFromDockerfile(b, ast); err != nil {
485+
return err
486+
}
487+
}
483488
return nil
484489
}
485490

@@ -644,8 +649,8 @@ func (b *Builder) clearTmp() {
644649
}
645650
}
646651

647-
// readDockerfile reads a Dockerfile from the current context.
648-
func (b *Builder) readDockerfile() (*parser.Node, error) {
652+
// readAndParseDockerfile reads a Dockerfile from the current context.
653+
func (b *Builder) readAndParseDockerfile() (*parser.Node, error) {
649654
// If no -f was specified then look for 'Dockerfile'. If we can't find
650655
// that then look for 'dockerfile'. If neither are found then default
651656
// back to 'Dockerfile' and use that in the error message.

‎builder/dockerfile/internals_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,6 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath,
7777

7878
b := &Builder{options: options, context: context}
7979

80-
_, err = b.readDockerfile()
80+
_, err = b.readAndParseDockerfile()
8181
assert.Error(t, err, expectedError)
8282
}

0 commit comments

Comments
 (0)