From 8e39c2e75ac062c4fea1ce34709cb94bd799e673 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 24 Apr 2024 11:30:22 +0200 Subject: [PATCH 1/5] Allow build context to be modified when Dockerfile is used outside the base workspace dir Signed-off-by: Danny Kopping --- envbuilder.go | 6 ++- integration/integration_test.go | 85 +++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/envbuilder.go b/envbuilder.go index 5a85f306..44da8cc6 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -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 a relative path since it will be joined to the WorkspaceFolder path. + 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"` @@ -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), } } } diff --git a/integration/integration_test.go b/integration/integration_test.go index ba63ac6f..fba06b69 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -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 scratch +COPY %s .`, 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. + 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() From c8f502e98147e56c8c53a600b4c2d66116739adb Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 24 Apr 2024 12:38:26 +0200 Subject: [PATCH 2/5] Use alpine image Signed-off-by: Danny Kopping --- integration/integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/integration_test.go b/integration/integration_test.go index fba06b69..9b3a0e30 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -723,8 +723,8 @@ func TestDockerfileBuildContext(t *testing.T) { t.Parallel() inclFile := "myfile" - dockerfile := fmt.Sprintf(`FROM scratch -COPY %s .`, inclFile) + dockerfile := fmt.Sprintf(`FROM %s +COPY %s .`, testImageAlpine, inclFile) tests := []struct { name string From 21e2081fadea7418065f93a95bbe2820d3e5f80a Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 24 Apr 2024 12:38:42 +0200 Subject: [PATCH 3/5] Add race test target Signed-off-by: Danny Kopping --- Makefile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Makefile b/Makefile index 33a4ddbb..7d6102ce 100644 --- a/Makefile +++ b/Makefile @@ -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 From 0643d173ef0ad63dae5a7ea3697f16891564b378 Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 24 Apr 2024 12:41:21 +0200 Subject: [PATCH 4/5] Wording Signed-off-by: Danny Kopping --- envbuilder.go | 2 +- integration/integration_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/envbuilder.go b/envbuilder.go index 44da8cc6..19d708bf 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -140,7 +140,7 @@ type Options struct { DockerfilePath string `env:"DOCKERFILE_PATH"` // BuildContextPath can be specified when a DockerfilePath is specified outside the base WorkspaceFolder. - // This path MUST be a relative path since it will be joined to the WorkspaceFolder path. + // 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 diff --git a/integration/integration_test.go b/integration/integration_test.go index 9b3a0e30..3fbed8c7 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -758,7 +758,7 @@ COPY %s .`, testImageAlpine, inclFile) }, { // 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 should allow inclFile to be copied. This is probably not desirable though? name: "different build context (default, different content roots)", files: map[string]string{ "a/Dockerfile": dockerfile, From 73e8800707ad038c7ab2bc60c2a9a640d69bc2eb Mon Sep 17 00:00:00 2001 From: Danny Kopping Date: Wed, 24 Apr 2024 14:12:47 +0200 Subject: [PATCH 5/5] Add warning Signed-off-by: Danny Kopping --- envbuilder.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/envbuilder.go b/envbuilder.go index 19d708bf..e91157c9 100644 --- a/envbuilder.go +++ b/envbuilder.go @@ -471,6 +471,15 @@ func Run(ctx context.Context, options Options) error { } else { // If a Dockerfile was specified, we use that. dockerfilePath := filepath.Join(options.WorkspaceFolder, options.DockerfilePath) + + // If the dockerfilePath is specified and deeper than the base of WorkspaceFolder AND the BuildContextPath is + // not defined, show a warning + dockerfileDir := filepath.Dir(dockerfilePath) + if dockerfileDir != filepath.Clean(options.WorkspaceFolder) && options.BuildContextPath == "" { + logf(codersdk.LogLevelWarn, "given dockerfile %q is below %q and no custom build context has been defined", dockerfilePath, options.WorkspaceFolder) + logf(codersdk.LogLevelWarn, "\t-> set BUILD_CONTEXT_PATH to %q to fix", dockerfileDir) + } + dockerfile, err := options.Filesystem.Open(dockerfilePath) if err == nil { content, err := io.ReadAll(dockerfile)