Skip to content

Commit

Permalink
fix: missing image build errors
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
stevenh committed Jul 19, 2024
1 parent 70b90cc commit 44a5ea5
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 27 deletions.
23 changes: 11 additions & 12 deletions container_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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",
Expand All @@ -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",
},
}

Expand All @@ -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)
})
}
}
Expand Down
26 changes: 12 additions & 14 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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

Expand Down
23 changes: 23 additions & 0 deletions from_dockerfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io"
"log"
"path/filepath"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion generic.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down

0 comments on commit 44a5ea5

Please sign in to comment.