Skip to content

Commit

Permalink
Fix a panic that could occur when context was omitted (#83)
Browse files Browse the repository at this point in the history
I could have sworn the context property was required, but evidently it
isn't and we weren't handling the case when it was missing.

This PR updates things to set a default location of the current
directory if the context is absent. Some unit tests are also added.

Fixes #78.
  • Loading branch information
blampe authored May 31, 2024
1 parent 44e082a commit 4e8cf8f
Show file tree
Hide file tree
Showing 27 changed files with 137 additions and 66 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Fixed the default value for `ACTIONS_CACHE_URL` when using GitHub action caching. (https://github.com/pulumi/pulumi-docker-build/pull/80)
- Fixed Java SDK publishing. (https://github.com/pulumi/pulumi-docker-build/pull/89)
- Fixed a panic that could occur when `context` was omitted. (https://github.com/pulumi/pulumi-docker-build/pull/83)

## 0.0.2 (2024-04-25)

Expand Down
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ define example
--language $(1) \
--out ${WORKING_DIR}/examples/$(1)
cp -r ${WORKING_DIR}/examples/app ${WORKING_DIR}/examples/$(1)/app
cp ${WORKING_DIR}/examples/yaml/.dockerignore ${WORKING_DIR}/examples/$(1)/.dockerignore
endef

up::
Expand Down
2 changes: 2 additions & 0 deletions examples/dotnet/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
4 changes: 0 additions & 4 deletions examples/dotnet/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -239,10 +239,6 @@ COPY hello.c ./
RUN echo ""This uses an inline Dockerfile! 👍""
",
},
Context = new DockerBuild.Inputs.BuildContextArgs
{
Location = "./app",
},
});

var dockerLoad = new DockerBuild.Image("dockerLoad", new()
Expand Down
2 changes: 2 additions & 0 deletions examples/go/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
3 changes: 0 additions & 3 deletions examples/go/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,6 @@ func main() {
Dockerfile: &dockerbuild.DockerfileArgs{
Inline: pulumi.String("FROM alpine\nRUN echo \"This uses an inline Dockerfile! 👍\"\n"),
},
Context: &dockerbuild.BuildContextArgs{
Location: pulumi.String("./app"),
},
})
if err != nil {
return err
Expand Down
2 changes: 2 additions & 0 deletions examples/java/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
3 changes: 0 additions & 3 deletions examples/java/src/main/java/generated_program/App.java
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,6 @@ public static void stack(Context ctx) {
RUN echo "This uses an inline Dockerfile! 👍"
""")
.build())
.context(BuildContextArgs.builder()
.location("./app")
.build())
.build());

var dockerLoad = new Image("dockerLoad", ImageArgs.builder()
Expand Down
2 changes: 2 additions & 0 deletions examples/nodejs/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
3 changes: 0 additions & 3 deletions examples/nodejs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,6 @@ const inline = new docker_build.Image("inline", {
RUN echo "This uses an inline Dockerfile! 👍"
`,
},
context: {
location: "./app",
},
});
const dockerLoad = new docker_build.Image("dockerLoad", {
push: false,
Expand Down
2 changes: 2 additions & 0 deletions examples/python/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
3 changes: 0 additions & 3 deletions examples/python/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,6 @@
inline="""FROM alpine
RUN echo "This uses an inline Dockerfile! 👍"
""",
),
context=docker_build.BuildContextArgs(
location="./app",
))
docker_load = docker_build.Image("dockerLoad",
push=False,
Expand Down
2 changes: 2 additions & 0 deletions examples/yaml/.dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
command-output
tmp
2 changes: 0 additions & 2 deletions examples/yaml/Pulumi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,6 @@ resources:
inline: |
FROM alpine
RUN echo "This uses an inline Dockerfile! 👍"
context:
location: "./app"
# docker buildx build --load .
dockerLoad:
Expand Down
4 changes: 2 additions & 2 deletions provider/cmd/pulumi-resource-docker-build/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@
},
"context": {
"$ref": "#/types/docker-build:index:BuildContext",
"description": "Build context settings.\n\nEquivalent to Docker's `PATH | URL | -` positional argument."
"description": "Build context settings. Defaults to the current directory.\n\nEquivalent to Docker's `PATH | URL | -` positional argument."
},
"contextHash": {
"type": "string",
Expand Down Expand Up @@ -1309,7 +1309,7 @@
},
"context": {
"$ref": "#/types/docker-build:index:BuildContext",
"description": "Build context settings.\n\nEquivalent to Docker's `PATH | URL | -` positional argument."
"description": "Build context settings. Defaults to the current directory.\n\nEquivalent to Docker's `PATH | URL | -` positional argument."
},
"dockerfile": {
"$ref": "#/types/docker-build:index:Dockerfile",
Expand Down
18 changes: 15 additions & 3 deletions provider/internal/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ import (
"os"
"path"
"path/filepath"
"slices"
"syscall"

buildx "github.com/docker/buildx/build"
"github.com/moby/patternmatcher/ignorefile"
"github.com/spf13/afero"
"github.com/tonistiigi/fsutil"
"golang.org/x/exp/maps"

"github.com/pulumi/pulumi-go-provider/infer"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
Expand Down Expand Up @@ -90,7 +92,9 @@ func (c *Context) Annotate(a infer.Annotator) {

// validate returns a non-nil CheckError if the Context is invalid. The
// returned Dockerfile may have defaults set to match Docker's default
// handling. The returned Dockerfile should be validated separately.
// handling. The returned Dockerfile should be validated separately. Non-nil
// values are returned even in the case of errors to allow additional
// validation to be performed.
func (bc *BuildContext) validate(preview bool, d *Dockerfile) (*Dockerfile, *Context, error) {
if d == nil {
d = &Dockerfile{}
Expand All @@ -107,6 +111,11 @@ func (bc *BuildContext) validate(preview bool, d *Dockerfile) (*Dockerfile, *Con
// a build later.
return d, c, nil
}
// If this isn't a preview but our location still isn't set, default it to
// the current directory.
if c.Location == "" {
c.Location = "."
}

if buildx.IsRemoteURL(c.Location) {
// We assume remote URLs are always valid.
Expand Down Expand Up @@ -225,8 +234,11 @@ func hashBuildContext(
}
}

// Hash any local named contexts.
for _, namedContext := range namedContexts {
// Hash any local named contexts. Sort keys for stable iteration order.
keys := maps.Keys(namedContexts)
slices.Sort(keys)
for _, key := range keys {
namedContext := namedContexts[key]
if isLocalDir(fs, namedContext) {
fs, err := rootFS(namedContext, excludes)
if err != nil {
Expand Down
39 changes: 24 additions & 15 deletions provider/internal/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,71 +33,77 @@ func TestValidateContext(t *testing.T) {
t.Parallel()
tests := []struct {
name string
c Context
c *BuildContext
givenD Dockerfile
preview bool

wantD *Dockerfile
wantC *Context
wantErr string
}{
{
name: "relative",
c: Context{
c: &BuildContext{Context: Context{
Location: "../internal/../internal/testdata/noop",
},
}},
wantD: &Dockerfile{
Location: "../internal/testdata/noop/Dockerfile",
},
},
{
name: "missing directory",
c: Context{
c: &BuildContext{Context: Context{
Location: "/does/not/exist/",
},
}},
wantErr: "not a valid directory",
},
{
name: "missing default Dockerfile",
c: Context{
c: &BuildContext{Context: Context{
Location: "testdata",
},
}},
wantD: &Dockerfile{Location: "testdata/Dockerfile"},
},
{
name: "with explicit Dockerfile",
c: Context{
c: &BuildContext{Context: Context{
Location: "testdata",
},
}},
givenD: Dockerfile{
Location: "testdata/Dockerfile.invalid",
},
},
{
name: "default location",
c: Context{},
c: &BuildContext{Context: Context{}},
wantD: &Dockerfile{Location: "Dockerfile"},
},
{
name: "remote context doesn't default to local Dockerfile",
c: Context{
c: &BuildContext{Context: Context{
Location: "https://raw.githubusercontent.com/pulumi/pulumi-docker/api-types/provider/testdata/Dockerfile",
},
}},
wantD: &Dockerfile{},
},
{
name: "preview",
c: Context{},
c: &BuildContext{Context: Context{}},
preview: true,
},
{
name: "missing context defaults to current directory",
c: nil,
wantC: &Context{Location: "."},
wantD: &Dockerfile{Location: "Dockerfile"},
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

bc := &BuildContext{Context: tt.c}
d, _, err := bc.validate(tt.preview, &tt.givenD)
d, c, err := tt.c.validate(tt.preview, &tt.givenD)

if tt.wantErr == "" {
assert.NoError(t, err)
Expand All @@ -109,6 +115,9 @@ func TestValidateContext(t *testing.T) {
assert.Equal(t, tt.wantD.Location, d.Location)
assert.Equal(t, tt.wantD.Inline, d.Inline)
}
if tt.wantC != nil {
assert.Equal(t, tt.wantC.Location, c.Location)
}
})
}
}
Expand Down
18 changes: 13 additions & 5 deletions provider/internal/host.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package internal

import (
"context"
"fmt"
"path/filepath"
"sync"
"time"

"github.com/docker/buildx/builder"
"github.com/docker/buildx/store/storeutil"
Expand Down Expand Up @@ -72,7 +74,7 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {

txn, release, err := storeutil.GetStore(h.cli)
if err != nil {
return nil, err
return nil, fmt.Errorf("getting store: %w", err)
}
defer release()

Expand All @@ -86,7 +88,7 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {
builder.WithStore(txn),
)
if err != nil {
return nil, err
return nil, fmt.Errorf("new builder: %w", err)
}

// If we didn't request a particular builder, and we loaded a default
Expand All @@ -95,7 +97,7 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {
if b.Driver == "" && opts.Builder == "" {
builders, err := builder.GetBuilders(h.cli, txn)
if err != nil {
return nil, err
return nil, fmt.Errorf("getting builders: %w", err)
}
nextbuilder:
for _, bb := range builders {
Expand Down Expand Up @@ -128,6 +130,7 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {
}

if b.Driver == "" && opts.Builder == "" {

// If we STILL don't have a builder, create a docker-container instance.
b, err = builder.Create(
context.Background(),
Expand All @@ -136,7 +139,12 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {
builder.CreateOpts{Driver: "docker-container"},
)
if err != nil {
return nil, err
return nil, fmt.Errorf("creating builder: %w", err)
}
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if _, err := b.Boot(ctx); err != nil {
return nil, fmt.Errorf("booting builder: %w", err)
}
}

Expand All @@ -145,7 +153,7 @@ func (h *host) builderFor(build Build) (*cachedBuilder, error) {
// drivers that are unknown to us.
nodes, err := b.LoadNodes(context.Background())
if err != nil && !build.ShouldExec() {
return nil, err
return nil, fmt.Errorf("loading nodes: %w", err)
}

cached := &cachedBuilder{name: b.Name, driver: b.Driver, nodes: nodes}
Expand Down
6 changes: 5 additions & 1 deletion provider/internal/image.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ func (ia *ImageArgs) Annotate(a infer.Annotator) {
Equivalent to Docker's "--cache-to" flag.
`))
a.Describe(&ia.Context, dedent(`
Build context settings.
Build context settings. Defaults to the current directory.
Equivalent to Docker's "PATH | URL | -" positional argument.
`))
Expand Down Expand Up @@ -546,6 +546,10 @@ func (ia *ImageArgs) validate(preview bool) (controllerapi.BuildOptions, error)
multierr = errors.Join(multierr, err)
}
ia.Dockerfile = dockerfile
// Set a default context if one wasn't provided.
if ia.Context == nil {
ia.Context = &BuildContext{Context: *context}
}

if err := ia.Dockerfile.validate(preview, context); err != nil {
multierr = errors.Join(multierr, err)
Expand Down
Loading

0 comments on commit 4e8cf8f

Please sign in to comment.