From 01b25b13d9b78fef4b24f7c659e349a01ee3f488 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Jul 2023 10:03:39 +0200 Subject: [PATCH 1/3] frontend/dockerfile/dockerignore: cleanup unit test - don't use a temp-file for the test as all we need is a reader - use a const and string-literal for the test-content, which makes it slightly more readable - don't use hard-coded tests for each line, but use an "expected" slice - don't fail early if line-numbers don't match Signed-off-by: Sebastiaan van Stijn --- .../dockerignore/dockerignore_test.go | 77 +++++++++---------- 1 file changed, 35 insertions(+), 42 deletions(-) diff --git a/frontend/dockerfile/dockerignore/dockerignore_test.go b/frontend/dockerfile/dockerignore/dockerignore_test.go index c3cd07884cf8..86176fdff388 100644 --- a/frontend/dockerfile/dockerignore/dockerignore_test.go +++ b/frontend/dockerfile/dockerignore/dockerignore_test.go @@ -1,61 +1,54 @@ package dockerignore import ( - "os" - "path/filepath" + "strings" "testing" ) func TestReadAll(t *testing.T) { - di, err := ReadAll(nil) + actual, err := ReadAll(nil) if err != nil { - t.Fatalf("Expected not to have error, got %v", err) + t.Errorf("Expected no error, got %v", err) } - - if diLen := len(di); diLen != 0 { - t.Fatalf("Expected to have zero dockerignore entry, got %d", diLen) + if entries := len(actual); entries != 0 { + t.Fatalf("Expected to have zero entries, got %d", entries) } - diName := filepath.Join(t.TempDir(), ".dockerignore") - content := "test1\n/test2\n/a/file/here\n\nlastfile\n# this is a comment\n! /inverted/abs/path\n!\n! \n" - err = os.WriteFile(diName, []byte(content), 0600) - if err != nil { - t.Fatal(err) - } + const content = `test1 +/test2 +/a/file/here - diFd, err := os.Open(diName) - if err != nil { - t.Fatal(err) +lastfile +# this is a comment +! /inverted/abs/path +! +! ` + + expected := []string{ + "test1", + "test2", // according to https://docs.docker.com/engine/reference/builder/#dockerignore-file, /foo/bar should be treated as foo/bar + "a/file/here", // according to https://docs.docker.com/engine/reference/builder/#dockerignore-file, /foo/bar should be treated as foo/bar + "lastfile", + "!inverted/abs/path", + "!", + "!", } - defer diFd.Close() - di, err = ReadAll(diFd) + actual, err = ReadAll(strings.NewReader(content)) if err != nil { - t.Fatal(err) + t.Error(err) } - if len(di) != 7 { - t.Fatalf("Expected 7 entries, got %v", len(di)) - } - if di[0] != "test1" { - t.Fatal("First element is not test1") - } - if di[1] != "test2" { // according to https://docs.docker.com/engine/reference/builder/#dockerignore-file, /foo/bar should be treated as foo/bar - t.Fatal("Second element is not test2") - } - if di[2] != "a/file/here" { // according to https://docs.docker.com/engine/reference/builder/#dockerignore-file, /foo/bar should be treated as foo/bar - t.Fatal("Third element is not a/file/here") - } - if di[3] != "lastfile" { - t.Fatal("Fourth element is not lastfile") - } - if di[4] != "!inverted/abs/path" { - t.Fatal("Fifth element is not !inverted/abs/path") - } - if di[5] != "!" { - t.Fatalf("Sixth element is not !, but %s", di[5]) - } - if di[6] != "!" { - t.Fatalf("Seventh element is not !, but %s", di[6]) + if len(actual) != len(expected) { + t.Errorf("Expected %d entries, got %v", len(expected), len(actual)) + } + for i, expectedLine := range expected { + if i >= len(actual) { + t.Errorf(`missing line %d: expected: "%s", got none`, i+1, expectedLine) + continue + } + if actual[i] != expectedLine { + t.Errorf(`line %d: expected: "%s", got: "%s"`, i+1, expectedLine, actual[i]) + } } } From d11608118968a01cb7f91ab7c2ff9e9e3fe2920c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Jul 2023 10:26:14 +0200 Subject: [PATCH 2/3] frontend/dockerfile/dockerignore: touch-up godoc and code Use "doc links" where possible, and better describe the function. Signed-off-by: Sebastiaan van Stijn --- .../dockerfile/dockerignore/dockerignore.go | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/frontend/dockerfile/dockerignore/dockerignore.go b/frontend/dockerfile/dockerignore/dockerignore.go index e7f29ae8df68..c4406c414647 100644 --- a/frontend/dockerfile/dockerignore/dockerignore.go +++ b/frontend/dockerfile/dockerignore/dockerignore.go @@ -10,19 +10,29 @@ import ( "github.com/pkg/errors" ) -// ReadAll reads a .dockerignore file and returns the list of file patterns -// to ignore. Note this will trim whitespace from each line as well -// as use GO's "clean" func to get the shortest/cleanest path for each. +// ReadAll reads an ignore file from a reader and returns the list of file +// patterns to ignore, applying the following rules: +// +// - An UTF8 BOM header (if present) is stripped. +// - Lines starting with "#" are considered comments and are skipped. +// +// For remaining lines: +// +// - Leading and trailing whitespace is removed from each ignore pattern. +// - It uses [filepath.Clean] to get the shortest/cleanest path for +// ignore patterns. +// - Leading forward-slashes ("/") are removed from ignore patterns, +// so "/some/path" and "some/path" are considered equivalent. func ReadAll(reader io.Reader) ([]string, error) { if reader == nil { return nil, nil } - scanner := bufio.NewScanner(reader) var excludes []string currentLine := 0 - utf8bom := []byte{0xEF, 0xBB, 0xBF} + + scanner := bufio.NewScanner(reader) for scanner.Scan() { scannedBytes := scanner.Bytes() // We trim UTF8 BOM From 7eb2ceafc3f3a5e601ab8759244d7d6144d92329 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Jul 2023 10:32:19 +0200 Subject: [PATCH 3/3] frontend/dockerfile/dockerignore: remove hard-coded filename from error While this function would usually be used for read a `.dockerignore` file, it accepts a Reader and can also be used to handle ignore patterns from other files (e.g. `Dockerfile.dockerignore`) or other sources. The error was also wrapped multiple times in some code-paths, which could lead to an error being formatted as: failed to parse dockerignore: error reading .dockerignore: Let's remove mention of the `.dockerignore` filename from the error, and leave it to the caller to include the filename. This patch also brings the MainContext dockerignore error inline with the NamedContext dockerignore error, now printing the exact name of the file. Co-authored-by: Justin Chadwell Signed-off-by: Sebastiaan van Stijn --- frontend/dockerfile/dockerignore/dockerignore.go | 4 +--- frontend/dockerui/config.go | 7 +++++-- frontend/dockerui/namedcontext.go | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/frontend/dockerfile/dockerignore/dockerignore.go b/frontend/dockerfile/dockerignore/dockerignore.go index c4406c414647..2bd9f1c9e451 100644 --- a/frontend/dockerfile/dockerignore/dockerignore.go +++ b/frontend/dockerfile/dockerignore/dockerignore.go @@ -6,8 +6,6 @@ import ( "io" "path/filepath" "strings" - - "github.com/pkg/errors" ) // ReadAll reads an ignore file from a reader and returns the list of file @@ -69,7 +67,7 @@ func ReadAll(reader io.Reader) ([]string, error) { excludes = append(excludes, pattern) } if err := scanner.Err(); err != nil { - return nil, errors.Wrap(err, "error reading .dockerignore") + return nil, err } return excludes, nil } diff --git a/frontend/dockerui/config.go b/frontend/dockerui/config.go index 12ec2c6880e0..a83c49940b43 100644 --- a/frontend/dockerui/config.go +++ b/frontend/dockerui/config.go @@ -80,7 +80,8 @@ type Client struct { g flightcontrol.Group[*buildContext] bopts client.BuildOpts - dockerignore []byte + dockerignore []byte + dockerignoreName string } type SBOM struct { @@ -375,6 +376,7 @@ func (bc *Client) ReadEntrypoint(ctx context.Context, lang string, opts ...llb.L }) if err == nil { bc.dockerignore = dt + bc.dockerignoreName = bctx.filename + ".dockerignore" } return &Source{ @@ -435,13 +437,14 @@ func (bc *Client) MainContext(ctx context.Context, opts ...llb.LocalOption) (*ll dt = []byte{} } bc.dockerignore = dt + bc.dockerignoreName = DefaultDockerignoreName } var excludes []string if len(bc.dockerignore) != 0 { excludes, err = dockerignore.ReadAll(bytes.NewBuffer(bc.dockerignore)) if err != nil { - return nil, errors.Wrap(err, "failed to parse dockerignore") + return nil, errors.Wrapf(err, "failed parsing %s", bc.dockerignoreName) } } diff --git a/frontend/dockerui/namedcontext.go b/frontend/dockerui/namedcontext.go index 6a441c50822e..8f004baa92bd 100644 --- a/frontend/dockerui/namedcontext.go +++ b/frontend/dockerui/namedcontext.go @@ -208,7 +208,7 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi if len(dt) != 0 { excludes, err = dockerignore.ReadAll(bytes.NewBuffer(dt)) if err != nil { - return nil, nil, err + return nil, nil, errors.Wrapf(err, "failed parsing %s", DefaultDockerignoreName) } } }