Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow custom build context to be configured when using DOCKERFILE_PATH #139

Merged
merged 5 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ build: scripts/envbuilder-$(GOARCH)
test: test-registry test-images
go test -count=1 ./...

test-race:
go test -race -count=3 ./...

# Starts a local Docker registry on port 5000 with a local disk cache.
.PHONY: test-registry
test-registry: .registry-cache
Expand Down
6 changes: 5 additions & 1 deletion envbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ type Options struct {
// to using a devcontainer that some might find simpler.
DockerfilePath string `env:"DOCKERFILE_PATH"`

// BuildContextPath can be specified when a DockerfilePath is specified outside the base WorkspaceFolder.
// This path MUST be relative to the WorkspaceFolder path into which the repo is cloned.
BuildContextPath string `env:"BUILD_CONTEXT_PATH"`

// CacheTTLDays is the number of days to use cached layers before
// expiring them. Defaults to 7 days.
CacheTTLDays int `env:"CACHE_TTL_DAYS"`
Expand Down Expand Up @@ -476,7 +480,7 @@ func Run(ctx context.Context, options Options) error {
buildParams = &devcontainer.Compiled{
DockerfilePath: dockerfilePath,
DockerfileContent: string(content),
BuildContext: options.WorkspaceFolder,
BuildContext: filepath.Join(options.WorkspaceFolder, options.BuildContextPath),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since WorkspaceFolder is optional, what's the end-result if it's not provided, will this be correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WorkspaceFolder will be given a default value if none is provided:

envbuilder/envbuilder.go

Lines 270 to 276 in 0643d17

if options.WorkspaceFolder == "" {
var err error
options.WorkspaceFolder, err = DefaultWorkspaceFolder(options.GitURL)
if err != nil {
return err
}
}

}
}
}
Expand Down
85 changes: 85 additions & 0 deletions integration/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,91 @@ func TestNoMethodFails(t *testing.T) {
require.ErrorContains(t, err, envbuilder.ErrNoFallbackImage.Error())
}

func TestDockerfileBuildContext(t *testing.T) {
t.Parallel()

inclFile := "myfile"
dockerfile := fmt.Sprintf(`FROM %s
COPY %s .`, testImageAlpine, inclFile)

tests := []struct {
name string
files map[string]string
dockerfilePath string
buildContextPath string
expectedErr string
}{
{
// Dockerfile & build context are in the same dir, copying inclFile should work.
name: "same build context (default)",
files: map[string]string{
"Dockerfile": dockerfile,
inclFile: "...",
},
dockerfilePath: "Dockerfile",
buildContextPath: "", // use default
expectedErr: "", // expect no errors
},
{
// Dockerfile & build context are not in the same dir, build context is still the default; this should fail
// to copy inclFile since it is not in the same dir as the Dockerfile.
name: "different build context (default)",
files: map[string]string{
"a/Dockerfile": dockerfile,
"a/" + inclFile: "...",
},
dockerfilePath: "a/Dockerfile",
buildContextPath: "", // use default
expectedErr: inclFile + ": no such file or directory",
},
{
// Dockerfile & build context are not in the same dir, but inclFile is in the default build context dir;
// this should allow inclFile to be copied. This is probably not desirable though?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer: should we warn when a Dockerfile is used outside the WorkspaceFolder, because the build context will likely be wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also make an argument for failing the build entirely; but I think warning is fine here.

name: "different build context (default, different content roots)",
files: map[string]string{
"a/Dockerfile": dockerfile,
inclFile: "...",
},
dockerfilePath: "a/Dockerfile",
buildContextPath: "", // use default
expectedErr: "",
},
{
// Dockerfile is not in the default build context dir, but the build context has been overridden; this should
// allow inclFile to be copied.
name: "different build context (custom)",
files: map[string]string{
"a/Dockerfile": dockerfile,
"a/" + inclFile: "...",
},
dockerfilePath: "a/Dockerfile",
buildContextPath: "a/",
expectedErr: "",
},
}

for _, tc := range tests {
tc := tc

t.Run(tc.name, func(t *testing.T) {
url := createGitServer(t, gitServerOptions{
files: tc.files,
})
_, err := runEnvbuilder(t, options{env: []string{
"GIT_URL=" + url,
"DOCKERFILE_PATH=" + tc.dockerfilePath,
"BUILD_CONTEXT_PATH=" + tc.buildContextPath,
}})

if tc.expectedErr == "" {
require.NoError(t, err)
} else {
require.ErrorContains(t, err, tc.expectedErr)
}
})
}
}

// TestMain runs before all tests to build the envbuilder image.
func TestMain(m *testing.M) {
cleanOldEnvbuilders()
Expand Down
Loading