Specify in which line the Dockerfile parser failed #30039

Merged
merged 1 commit into from Jan 13, 2017

Projects

None yet

6 participants

@ripcurld0
Contributor
ripcurld0 commented Jan 10, 2017 edited

closes #30022

- What I did
Add the line number in the file in the Dockerfile parser error.

- How I did it
Apparently, the parser.Node holds the starting line of the command in the file.
Using it, it's easy to concatenate the line number where Docker fails to parse the Dockerfile.

- How to verify it
Run integration-cli tests:

TESTFLAGS='-check.f DockerSuite.TestBuildLineError*' make test-integration-cli

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@justincormack
Member

poked the CI to actually run the tests...

@ripcurld0 ripcurld0 changed the title from Specify line numbers in Dockerfile parse errors to Specify in which step the Dockerfile parser failed Jan 11, 2017
@justincormack
Member

This is definitely an improvement, but I would prefer the line number not the step number if possible, as if people have made errors in commenting or continuations it might be confusing. If that is really hard, printing the step might be better too?

@ripcurld0
Contributor

@justincormack cool. I think I can do that 👣

@ripcurld0
Contributor

@justincormack 🕺 done

@AkihiroSuda AkihiroSuda changed the title from Specify in which step the Dockerfile parser failed to Specify in which line the Dockerfile parser failed Jan 11, 2017
builder/dockerfile/builder.go
@@ -264,7 +264,8 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
total := len(b.dockerfile.Children)
for _, n := range b.dockerfile.Children {
if err := b.checkDispatch(n, false); err != nil {
- return "", err
+ // Specify in which line the parsing failed
+ return "", fmt.Errorf("Line %d: %s", n.StartLine, err)
@justincormack
justincormack Jan 11, 2017 Member

I think we should specifically say Dockerfile parse error line %d... so the error is self contained, as there could be other types of Dockerfile error in future.

@ripcurld0
ripcurld0 Jan 11, 2017 Contributor

Yes, that makes more sense.

@justincormack
Member

Thanks! LGTM.

cc @thaJeztah

builder/dockerfile/builder.go
@@ -264,7 +264,8 @@ func (b *Builder) build(stdout io.Writer, stderr io.Writer, out io.Writer) (stri
total := len(b.dockerfile.Children)
for _, n := range b.dockerfile.Children {
if err := b.checkDispatch(n, false); err != nil {
- return "", err
+ // Specify in which line the parsing failed
+ return "", fmt.Errorf("Dockerfile parse error line %d: %s", n.StartLine, err)
@cpuguy83
cpuguy83 Jan 12, 2017 Contributor

errors.Wrapf
Also the comment seems redundant here.

@ripcurld0
ripcurld0 Jan 12, 2017 Contributor

Thanks @cpuguy83 I will do these changes.

@duglin
Contributor
duglin commented Jan 12, 2017

LGTM

@ripcurld0 ripcurld0 Specify in which line the Dockerfile parser failed
Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
8f282cd
@cpuguy83

LGTM

@cpuguy83 cpuguy83 merged commit 597843e into docker:master Jan 13, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29504 has succeeded
Details
janky Jenkins build Docker-PRs 38093 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9164 has succeeded
Details
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment