From 7292a5b5791847854f151df9be1605f8653202a8 Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Tue, 16 Jul 2024 15:00:32 +0100 Subject: [PATCH] fix: missing image build errors Ensure build errors are returned to the caller when PrintBuildLog is not set to true. DisplayJSONMessagesStream and JSONMessage.Display performs processing of the stream to catch command failures, so needs to always be called. --- container_test.go | 23 +++++++++++------------ docker.go | 26 ++++++++++++-------------- from_dockerfile_test.go | 23 +++++++++++++++++++++++ generic.go | 2 +- testdata/error.Dockerfile | 4 ++++ 5 files changed, 51 insertions(+), 27 deletions(-) create mode 100644 testdata/error.Dockerfile diff --git a/container_test.go b/container_test.go index 1d25c4d661..0da1b967f8 100644 --- a/container_test.go +++ b/container_test.go @@ -151,7 +151,7 @@ func Test_BuildImageWithContexts(t *testing.T) { ContextArchive func() (io.Reader, error) ExpectedEchoOutput string Dockerfile string - ExpectedError error + ExpectedError string } testCases := []TestCase{ @@ -252,7 +252,7 @@ func Test_BuildImageWithContexts(t *testing.T) { ExpectedEchoOutput: "hi this is from the say_hi.sh file!", }, { - Name: "test buildling from a context on the filesystem", + Name: "test building from a context on the filesystem", ContextPath: "./testdata", Dockerfile: "echo.Dockerfile", ExpectedEchoOutput: "this is from the echo test Dockerfile", @@ -266,7 +266,7 @@ func Test_BuildImageWithContexts(t *testing.T) { ContextArchive: func() (io.Reader, error) { return nil, nil }, - ExpectedError: errors.New("you must specify either a build context or an image: failed to create container"), + ExpectedError: "create container: you must specify either a build context or an image", }, } @@ -292,16 +292,15 @@ func Test_BuildImageWithContexts(t *testing.T) { ContainerRequest: req, Started: true, }) - switch { - case testCase.ExpectedError != nil && err != nil: - if testCase.ExpectedError.Error() != err.Error() { - t.Fatalf("unexpected error: %s, was expecting %s", err.Error(), testCase.ExpectedError.Error()) - } - case err != nil: - t.Fatal(err) - default: - terminateContainerOnEnd(t, ctx, c) + + defer terminateContainerOnEnd(t, ctx, c) + + if testCase.ExpectedError != "" { + require.EqualError(t, err, testCase.ExpectedError) + return } + + require.NoError(t, err) }) } } diff --git a/docker.go b/docker.go index e89b9f4649..c3dfcb8da2 100644 --- a/docker.go +++ b/docker.go @@ -887,14 +887,14 @@ func (p *DockerProvider) BuildImage(ctx context.Context, img ImageBuildInfo) (st var err error buildOptions, err = img.BuildOptions() if err != nil { - return types.ImageBuildResponse{}, backoff.Permanent(err) + return types.ImageBuildResponse{}, backoff.Permanent(fmt.Errorf("build options: %w", err)) } defer tryClose(buildOptions.Context) // release resources in any case resp, err := p.client.ImageBuild(ctx, buildOptions.Context, buildOptions) if err != nil { if isPermanentClientError(err) { - return types.ImageBuildResponse{}, backoff.Permanent(err) + return types.ImageBuildResponse{}, backoff.Permanent(fmt.Errorf("build image: %w", err)) } return types.ImageBuildResponse{}, err } @@ -908,30 +908,28 @@ func (p *DockerProvider) BuildImage(ctx context.Context, img ImageBuildInfo) (st }, ) if err != nil { - return "", err + return "", err // Error is already wrapped. } defer resp.Body.Close() + output := io.Discard if img.ShouldPrintBuildLog() { - termFd, isTerm := term.GetFdInfo(os.Stderr) - err = jsonmessage.DisplayJSONMessagesStream(resp.Body, os.Stderr, termFd, isTerm, nil) - if err != nil { - return "", err - } + output = os.Stderr } - // need to read the response from Docker, I think otherwise the image - // might not finish building before continuing to execute here - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return "", err + // Always process the output, even if it is not printed + // to ensure that errors during the build process are + // correctly handled. + termFd, isTerm := term.GetFdInfo(output) + if err = jsonmessage.DisplayJSONMessagesStream(resp.Body, output, termFd, isTerm, nil); err != nil { + return "", fmt.Errorf("build image: %w", err) } // the first tag is the one we want return buildOptions.Tags[0], nil } -// CreateContainer fulfills a request for a container without starting it +// CreateContainer fulfils a request for a container without starting it func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerRequest) (Container, error) { var err error diff --git a/from_dockerfile_test.go b/from_dockerfile_test.go index ab25ae6dee..fc1d4052ea 100644 --- a/from_dockerfile_test.go +++ b/from_dockerfile_test.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log" + "path/filepath" "strings" "testing" "time" @@ -88,6 +89,28 @@ func TestBuildImageFromDockerfile_NoRepo(t *testing.T) { }) } +func TestBuildImageFromDockerfile_BuildError(t *testing.T) { + ctx := context.Background() + dockerClient, err := NewDockerClientWithOpts(ctx) + require.NoError(t, err) + + defer dockerClient.Close() + + req := ContainerRequest{ + FromDockerfile: FromDockerfile{ + Dockerfile: "error.Dockerfile", + Context: filepath.Join(".", "testdata"), + }, + } + _, err = GenericContainer(ctx, GenericContainerRequest{ + ProviderType: providerType, + ContainerRequest: req, + Started: true, + }) + + require.EqualError(t, err, `create container: build image: The command '/bin/sh -c exit 1' returned a non-zero code: 1`) +} + func TestBuildImageFromDockerfile_NoTag(t *testing.T) { provider, err := NewDockerProvider() if err != nil { diff --git a/generic.go b/generic.go index 65fec35c88..f0cda13407 100644 --- a/generic.go +++ b/generic.go @@ -74,7 +74,7 @@ func GenericContainer(ctx context.Context, req GenericContainerRequest) (Contain } if err != nil { // At this point `c` might not be nil. Give the caller an opportunity to call Destroy on the container. - return c, fmt.Errorf("%w: failed to create container", err) + return c, fmt.Errorf("create container: %w", err) } if req.Started && !c.IsRunning() { diff --git a/testdata/error.Dockerfile b/testdata/error.Dockerfile new file mode 100644 index 0000000000..1284e7285f --- /dev/null +++ b/testdata/error.Dockerfile @@ -0,0 +1,4 @@ +FROM docker.io/alpine + +RUN exit 1 +