From 66d3aedce3b3be599fd117b3e3521fada1038fad Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sat, 27 Jun 2026 18:13:52 -0500 Subject: [PATCH 01/13] refactor(devcontainer): split compose.go and reduce lint complexity Split the 1.7k-line compose.go into focused files (compose.go, compose_build.go, compose_up.go, compose_mounts.go) and decompose the large orchestration functions into testable helpers. - Replace per-call regex label escaping with a package-level strings.NewReplacer; hoist the COPY/ADD feature-folder regex to a precompiled package var. - Introduce param structs for functions exceeding the argument limit (startContainer, buildAndExtendDockerCompose, the run* dispatch siblings, Runner.Command, etc.). - Reduce cyclomatic/nesting complexity via focused helpers across compose.go, single.go, and container_tunnel.go. Behavior-preserving; 122 devcontainer + 76 cmd/internal tests pass. --- cmd/internal/container_tunnel.go | 47 +- pkg/devcontainer/build.go | 44 +- pkg/devcontainer/compose.go | 1483 ++++++++++------------------ pkg/devcontainer/compose_build.go | 615 ++++++++++++ pkg/devcontainer/compose_mounts.go | 86 ++ pkg/devcontainer/compose_test.go | 117 ++- pkg/devcontainer/compose_up.go | 238 +++++ pkg/devcontainer/run.go | 112 ++- pkg/devcontainer/single.go | 290 +++--- 9 files changed, 1852 insertions(+), 1180 deletions(-) create mode 100644 pkg/devcontainer/compose_build.go create mode 100644 pkg/devcontainer/compose_mounts.go create mode 100644 pkg/devcontainer/compose_up.go diff --git a/cmd/internal/container_tunnel.go b/cmd/internal/container_tunnel.go index ede6ecdfa..aa8879265 100644 --- a/cmd/internal/container_tunnel.go +++ b/cmd/internal/container_tunnel.go @@ -93,7 +93,13 @@ func (cmd *ContainerTunnelCmd) Run(ctx context.Context) error { stdin io.Reader, stdout, stderr io.Writer, ) error { - return runner.Command(ctx, user, command, stdin, stdout, stderr) + return runner.Command(ctx, devcontainer.CommandParams{ + User: user, + Command: command, + Stdin: stdin, + Stdout: stdout, + Stderr: stderr, + }) }, User: cmd.User, Stdin: os.Stdin, @@ -113,29 +119,36 @@ func startDevContainer( return err } - // start container if necessary + // start container if it is missing or not running if containerDetails == nil || containerDetails.State.Status != "running" { - // start container _, err = StartContainer(ctx, runner, workspaceConfig) - if err != nil { - return err - } - } else if encoding.IsLegacyUID(workspaceConfig.Workspace.UID) { - // make sure workspace result is in devcontainer - buf := &bytes.Buffer{} - err = runner.Command(ctx, "root", "cat "+pkgconfig.DevContainerResultPath, nil, buf, buf) - if err != nil { - // start container - _, err = StartContainer(ctx, runner, workspaceConfig) - if err != nil { - return err - } - } + return err + } + + // for legacy UIDs, ensure the workspace result is present in the container, + // restarting it when the result is missing + if encoding.IsLegacyUID(workspaceConfig.Workspace.UID) && + !hasDevContainerResult(ctx, runner) { + _, err = StartContainer(ctx, runner, workspaceConfig) + return err } return nil } +// hasDevContainerResult reports whether the devcontainer result file is readable +// inside the running container. +func hasDevContainerResult(ctx context.Context, runner devcontainer.Runner) bool { + buf := &bytes.Buffer{} + err := runner.Command(ctx, devcontainer.CommandParams{ + User: "root", + Command: "cat " + pkgconfig.DevContainerResultPath, + Stdout: buf, + Stderr: buf, + }) + return err == nil +} + func StartContainer( ctx context.Context, runner devcontainer.Runner, diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 5fd4560b5..5a5b06d2e 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -427,10 +427,7 @@ func (r *runner) buildDevImageCompose( return nil, fmt.Errorf("find docker compose: %w", err) } - envFiles, err := r.getEnvFiles() - if err != nil { - return nil, fmt.Errorf("get env files: %w", err) - } + envFiles := r.getEnvFiles() composeFiles, err := r.getDockerComposeFilePaths(parsedConfig, envFiles) if err != nil { @@ -454,33 +451,24 @@ func (r *runner) buildDevImageCompose( project.Name = composeHelper.GetProjectName(r.ID) log.Debugf("Loaded project %s", project.Name) - service := parsedConfig.Config.Service - composeService, err := project.GetService(service) - if err != nil { - return nil, fmt.Errorf( - "service %q configured in devcontainer.json not found in Docker Compose configuration", - service, - ) - } - - originalImageName := composeService.Image - if originalImageName == "" { - originalImageName, err = composeHelper.GetDefaultImage(project.Name, service) - if err != nil { - return nil, fmt.Errorf("get default image: %w", err) - } - } - - extendResult, err := r.buildAndExtendDockerCompose( - ctx, - parsedConfig, - substitutionContext, + composeService, originalImageName, err := resolveComposeServiceImage( project, composeHelper, - &composeService, - composeGlobalArgs, - options.FeatureSecretsFile, + parsedConfig.Config.Service, ) + if err != nil { + return nil, err + } + + extendResult, err := r.buildAndExtendDockerCompose(ctx, &buildAndExtendParams{ + parsedConfig: parsedConfig, + substitutionContext: substitutionContext, + project: project, + composeHelper: composeHelper, + composeService: &composeService, + globalArgs: composeGlobalArgs, + featureSecretsFile: options.FeatureSecretsFile, + }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) } diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 157947ac1..06b8e9c2c 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -6,28 +6,24 @@ import ( "os" "path" "path/filepath" - "reflect" - "regexp" - "strconv" "strings" - "time" composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/devcontainer/feature" "github.com/devsy-org/devsy/pkg/devcontainer/metadata" - "github.com/devsy-org/devsy/pkg/dockerfile" "github.com/devsy-org/devsy/pkg/driver" "github.com/devsy-org/devsy/pkg/log" "github.com/joho/godotenv" - "gopkg.in/yaml.v3" ) const ( ConfigFilesLabel = "com.docker.compose.project.config_files" FeaturesBuildOverrideFilePrefix = "docker-compose.devcontainer.build" FeaturesStartOverrideFilePrefix = "docker-compose.devcontainer.containerFeatures" + + containerStatusRunning = "running" + composeProjectNameFlag = "--project-name" ) type composeProjectFiles struct { @@ -55,6 +51,43 @@ type persistedFileResult struct { filePath string } +// startContainerParams groups the inputs for starting (or recreating) the +// compose dev container. ctx is passed separately to keep it out of the struct. +type startContainerParams struct { + parsedConfig *config.SubstitutedConfig + substitutionContext *config.SubstitutionContext + project *composetypes.Project + composeHelper *compose.ComposeHelper + composeGlobalArgs []string + container *config.ContainerDetails + options UpOptions +} + +// buildAndExtendParams groups the inputs for building and feature-extending a +// compose service. +type buildAndExtendParams struct { + parsedConfig *config.SubstitutedConfig + substitutionContext *config.SubstitutionContext + project *composetypes.Project + composeHelper *compose.ComposeHelper + composeService *composetypes.ServiceConfig + globalArgs []string + featureSecretsFile string +} + +// composeUpParams groups the inputs shared by extendedDockerComposeUp and +// generateDockerComposeUpProject for producing the compose "up" override. +type composeUpParams struct { + parsedConfig *config.SubstitutedConfig + mergedConfig *config.MergedDevContainerConfig + composeHelper *compose.ComposeHelper + composeService *composetypes.ServiceConfig + originalImageName string + overrideImageName string + imageDetails *config.ImageDetails + additionalLabels map[string]string +} + func (r *runner) composeHelper() (*compose.ComposeHelper, error) { dockerDriver, ok := r.Driver.(driver.DockerDriver) if !ok { @@ -121,10 +154,7 @@ func (r *runner) deleteDockerCompose( func (r *runner) dockerComposeProjectFiles( parsedConfig *config.SubstitutedConfig, ) (composeProjectFiles, error) { - envFiles, err := r.getEnvFiles() - if err != nil { - return composeProjectFiles{}, fmt.Errorf("get env files: %w", err) - } + envFiles := r.getEnvFiles() composeFiles, err := r.getDockerComposeFilePaths(parsedConfig, envFiles) if err != nil { @@ -149,11 +179,10 @@ func (r *runner) dockerComposeProjectFiles( func (r *runner) runDockerCompose( ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - options UpOptions, - timeout time.Duration, + runParams *runContainerParams, ) (*config.Result, error) { + parsedConfig := runParams.parsedConfig + composeHelper, err := r.composeHelper() if err != nil { return nil, fmt.Errorf("find docker compose: %w", err) @@ -163,8 +192,33 @@ func (r *runner) runDockerCompose( if err != nil { return nil, fmt.Errorf("get compose/env files: %w", err) } - composeGlobalArgs := projFiles.composeGlobalArgs + project, err := r.loadComposeProject(ctx, composeHelper, parsedConfig, projFiles) + if err != nil { + return nil, err + } + + containerDetails, err := r.ensureComposeContainer(ctx, &composeContainerParams{ + runParams: runParams, + composeHelper: composeHelper, + project: project, + composeGlobalArgs: projFiles.composeGlobalArgs, + }) + if err != nil { + return nil, err + } + + return r.finalizeComposeContainer(ctx, runParams, project, containerDetails) +} + +// loadComposeProject loads the docker compose project from the resolved compose +// and env files and names it after the workspace. +func (r *runner) loadComposeProject( + ctx context.Context, + composeHelper *compose.ComposeHelper, + parsedConfig *config.SubstitutedConfig, + projFiles composeProjectFiles, +) (*composetypes.Project, error) { log.Debugf("Loading docker compose project %+v", projFiles.composeFiles) project, err := compose.LoadDockerComposeProject( ctx, @@ -181,6 +235,30 @@ func (r *runner) runDockerCompose( return nil, err } + return project, nil +} + +// composeContainerParams groups the inputs for ensuring a running compose dev +// container. +type composeContainerParams struct { + runParams *runContainerParams + composeHelper *compose.ComposeHelper + project *composetypes.Project + composeGlobalArgs []string +} + +// ensureComposeContainer finds the dev container and, when it is missing, +// stopped, or being recreated, starts it (reusing persisted project files when +// possible). It returns the resolved container details. +func (r *runner) ensureComposeContainer( + ctx context.Context, + params *composeContainerParams, +) (*config.ContainerDetails, error) { + parsedConfig := params.runParams.parsedConfig + options := params.runParams.options + composeHelper := params.composeHelper + project := params.project + containerDetails, err := composeHelper.FindDevContainer( ctx, project.Name, @@ -190,73 +268,54 @@ func (r *runner) runDockerCompose( return nil, fmt.Errorf("find dev container: %w", err) } - // does the container already exist or is it not running? - if containerDetails == nil || containerDetails.State.Status != "running" || options.Recreate { - didStartProject := false - // Try to find existing project first - existingProjectFiles, err := composeHelper.FindProjectFiles(ctx, project.Name) - if err != nil { - log.Errorf("Error finding project files: %s", err) - } else if len(existingProjectFiles) > 0 && !options.Recreate { - log.Debugf("Found existing project files: %s", existingProjectFiles) - // make sure all project files are still available - for _, file := range existingProjectFiles { - if _, err := os.Stat(file); err != nil { - log.Warnf("Project file %s does not exist anymore, recreating project", file) - containerDetails = nil - break - } - } - - // If project is found, we can call `up` with the project name - // If it fails, fall back to rebuilding - upArgs := []string{"--project-name", project.Name} - for _, existingProjectFiles := range existingProjectFiles { - upArgs = append(upArgs, "-f", existingProjectFiles) - } - upArgs = append(upArgs, "up", "-d") - upArgs = r.onlyRunServices(upArgs, parsedConfig) - - // Run docker-compose - writer := log.Writer(log.LevelInfo) - err = composeHelper.Run(ctx, upArgs, nil, writer, writer) - if err != nil { - log.Errorf("Error starting project: %s", err) - } else { - // wait for running and get container details - details, err := composeHelper.FindDevContainer( - ctx, - project.Name, - parsedConfig.Config.Service, - ) - if err != nil { - log.Errorf("Error finding dev container: %s", err) - } else { - containerDetails = details - didStartProject = true - } - } - } + // container already exists and is running, nothing to do + if containerDetails != nil && containerDetails.State.Status == containerStatusRunning && + !options.Recreate { + return containerDetails, nil + } - // Start container if not running - if !didStartProject { - containerDetails, err = r.startContainer( - ctx, - parsedConfig, - substitutionContext, - project, - composeHelper, - composeGlobalArgs, - containerDetails, - options, - ) - if err != nil { - return nil, fmt.Errorf("start container: %w", err) - } else if containerDetails == nil { - return nil, fmt.Errorf("couldn't find container after start") - } - } + containerDetails, didStartProject := r.tryStartExistingProject(ctx, &existingProjectParams{ + parsedConfig: parsedConfig, + composeHelper: composeHelper, + project: project, + container: containerDetails, + recreate: options.Recreate, + }) + if didStartProject { + return containerDetails, nil + } + + containerDetails, err = r.startContainer(ctx, &startContainerParams{ + parsedConfig: parsedConfig, + substitutionContext: params.runParams.substitutionContext, + project: project, + composeHelper: composeHelper, + composeGlobalArgs: params.composeGlobalArgs, + container: containerDetails, + options: options, + }) + if err != nil { + return nil, fmt.Errorf("start container: %w", err) } + if containerDetails == nil { + return nil, fmt.Errorf("couldn't find container after start") + } + + return containerDetails, nil +} + +// finalizeComposeContainer derives the merged config from container metadata, +// updates the container user, exposes the compose project name, validates host +// requirements, and sets up the container. +func (r *runner) finalizeComposeContainer( + ctx context.Context, + runParams *runContainerParams, + project *composetypes.Project, + containerDetails *config.ContainerDetails, +) (*config.Result, error) { + parsedConfig := runParams.parsedConfig + substitutionContext := runParams.substitutionContext + options := runParams.options imageMetadataConfig, err := metadata.GetImageMetadataFromContainer( containerDetails, @@ -266,38 +325,16 @@ func (r *runner) runDockerCompose( return nil, fmt.Errorf("get image metadata from container: %w", err) } - if dockerDriver, ok := r.Driver.(driver.DockerDriver); ok { - err = dockerDriver.UpdateContainerUserUID( - ctx, - r.ID, - parsedConfig.Config, - log.Writer(log.LevelInfo), - ) - if err != nil { - log.Errorf("failed to update container user UID/GID: error=%v", err) - return nil, err - } - } - - if options.ExtraDevContainerPath != "" { - if imageMetadataConfig == nil { - imageMetadataConfig = &config.ImageMetadataConfig{} - } - extraConfig, parseErr := config.ParseDevContainerJSONFile(options.ExtraDevContainerPath) - if parseErr != nil { - return nil, parseErr - } - config.AddConfigToImageMetadata(extraConfig, imageMetadataConfig) + if err := r.updateContainerUserUID(ctx, parsedConfig); err != nil { + return nil, err } - mergedConfig, err := config.MergeConfiguration(parsedConfig.Config, imageMetadataConfig.Config) + mergedConfig, err := mergeImageMetadataConfig( + parsedConfig, + imageMetadataConfig, + options.ExtraDevContainerPath, + ) if err != nil { - return nil, fmt.Errorf("merge config: %w", err) - } - - if err := config.MergeExtraRemoteEnv( - mergedConfig, options.ExtraDevContainerPath, - ); err != nil { return nil, err } @@ -310,6 +347,51 @@ func (r *runner) runDockerCompose( composeAlias := project.Name mergedConfig.RemoteEnv["COMPOSE_PROJECT_NAME"] = &composeAlias + hostWarnings, err := r.composeHostWarnings(parsedConfig, substitutionContext, options) + if err != nil { + return nil, err + } + + return r.setupContainer(ctx, &setupContainerParams{ + rawConfig: parsedConfig.Raw, + containerDetails: containerDetails, + mergedConfig: mergedConfig, + substitutionContext: substitutionContext, + timeout: runParams.timeout, + hostWarnings: hostWarnings, + }) +} + +// updateContainerUserUID updates the container user's UID/GID to match the local +// user when running on a Docker driver. +func (r *runner) updateContainerUserUID( + ctx context.Context, + parsedConfig *config.SubstitutedConfig, +) error { + dockerDriver, ok := r.Driver.(driver.DockerDriver) + if !ok { + return nil + } + if err := dockerDriver.UpdateContainerUserUID( + ctx, + r.ID, + parsedConfig.Config, + log.Writer(log.LevelInfo), + ); err != nil { + log.Errorf("failed to update container user UID/GID: error=%v", err) + return err + } + return nil +} + +// composeHostWarnings validates host requirements, returning warnings. When the +// requirements are unmet it errors unless SkipHostRequirements is set, in which +// case the error is downgraded to a warning. +func (r *runner) composeHostWarnings( + parsedConfig *config.SubstitutedConfig, + substitutionContext *config.SubstitutionContext, + options UpOptions, +) ([]string, error) { hostWarnings, hostErr := config.ValidateHostRequirements( parsedConfig.Config.HostRequirements, config.SystemHostInfo{}, @@ -321,15 +403,84 @@ func (r *runner) runDockerCompose( } hostWarnings = append(hostWarnings, hostErr.Error()) } + return hostWarnings, nil +} - return r.setupContainer(ctx, &setupContainerParams{ - rawConfig: parsedConfig.Raw, - containerDetails: containerDetails, - mergedConfig: mergedConfig, - substitutionContext: substitutionContext, - timeout: timeout, - hostWarnings: hostWarnings, - }) +// existingProjectParams groups the inputs for attempting to start a compose +// project from previously persisted project files. +type existingProjectParams struct { + parsedConfig *config.SubstitutedConfig + composeHelper *compose.ComposeHelper + project *composetypes.Project + container *config.ContainerDetails + recreate bool +} + +// tryStartExistingProject attempts to bring up the dev container from project +// files discovered for an existing compose project, avoiding a full rebuild. It +// returns the (possibly updated) container details and whether the project was +// started. A false return means the caller should fall back to startContainer. +func (r *runner) tryStartExistingProject( + ctx context.Context, + params *existingProjectParams, +) (*config.ContainerDetails, bool) { + composeHelper := params.composeHelper + project := params.project + containerDetails := params.container + + existingProjectFiles, err := composeHelper.FindProjectFiles(ctx, project.Name) + if err != nil { + log.Errorf("Error finding project files: %s", err) + return containerDetails, false + } + if len(existingProjectFiles) == 0 || params.recreate { + return containerDetails, false + } + + log.Debugf("Found existing project files: %s", existingProjectFiles) + if !allProjectFilesExist(existingProjectFiles) { + containerDetails = nil + } + + // If project is found, we can call `up` with the project name. + // If it fails, fall back to rebuilding. + upArgs := []string{composeProjectNameFlag, project.Name} + for _, projectFile := range existingProjectFiles { + upArgs = append(upArgs, "-f", projectFile) + } + upArgs = append(upArgs, "up", "-d") + upArgs = r.onlyRunServices(upArgs, params.parsedConfig) + + writer := log.Writer(log.LevelInfo) + if err := composeHelper.Run(ctx, upArgs, nil, writer, writer); err != nil { + log.Errorf("Error starting project: %s", err) + return containerDetails, false + } + + // wait for running and get container details + details, err := composeHelper.FindDevContainer( + ctx, + project.Name, + params.parsedConfig.Config.Service, + ) + if err != nil { + log.Errorf("Error finding dev container: %s", err) + return containerDetails, false + } + + return details, true +} + +// allProjectFilesExist reports whether every persisted project file is still +// present on disk; a missing file means the project must be recreated. +func allProjectFilesExist(projectFiles []string) bool { + for _, file := range projectFiles { + if _, err := os.Stat(file); err != nil { + log.Warnf("Project file %s does not exist anymore, recreating project", file) + return false + } + } + return true } func validateRunServices(runServices []string, project *composetypes.Project) error { @@ -368,71 +519,81 @@ func (r *runner) getDockerComposeFilePaths( parsedConfig *config.SubstitutedConfig, envFiles []string, ) ([]string, error) { - configFileDir := filepath.Dir(parsedConfig.Config.Origin) - - // Use docker compose files from config - var composeFiles []string + // Prefer docker compose files declared in the devcontainer config. if len(parsedConfig.Config.DockerComposeFile) > 0 { - for _, composeFile := range parsedConfig.Config.DockerComposeFile { - absPath := composeFile - if !filepath.IsAbs(composeFile) { - absPath = filepath.Join(configFileDir, composeFile) - } - composeFiles = append(composeFiles, absPath) - } - - return composeFiles, nil + return absoluteComposeFiles( + filepath.Dir(parsedConfig.Config.Origin), + parsedConfig.Config.DockerComposeFile, + ), nil } - // Use docker compose files from $COMPOSE_FILE environment variable - envComposeFile := os.Getenv("COMPOSE_FILE") + // Otherwise fall back to $COMPOSE_FILE from the environment or .env files. + envComposeFile, err := composeFileFromEnv(envFiles) + if err != nil { + return nil, err + } + if envComposeFile != "" { + return filepath.SplitList(envComposeFile), nil + } - // Load docker compose files from $COMPOSE_FILE in .env file - if envComposeFile == "" { - for _, envFile := range envFiles { - env, err := godotenv.Read(envFile) - if err != nil { - return nil, err - } + return nil, nil +} - if env["COMPOSE_FILE"] != "" { - envComposeFile = env["COMPOSE_FILE"] - break - } +// absoluteComposeFiles resolves each compose file path against configFileDir +// unless it is already absolute. +func absoluteComposeFiles(configFileDir string, composeFiles []string) []string { + resolved := make([]string, 0, len(composeFiles)) + for _, composeFile := range composeFiles { + absPath := composeFile + if !filepath.IsAbs(composeFile) { + absPath = filepath.Join(configFileDir, composeFile) } + resolved = append(resolved, absPath) } + return resolved +} - if envComposeFile != "" { - return filepath.SplitList(envComposeFile), nil +// composeFileFromEnv returns the $COMPOSE_FILE value, preferring the process +// environment and falling back to the first .env file that defines it. +func composeFileFromEnv(envFiles []string) (string, error) { + if envComposeFile := os.Getenv("COMPOSE_FILE"); envComposeFile != "" { + return envComposeFile, nil } - return nil, nil + for _, envFile := range envFiles { + env, err := godotenv.Read(envFile) + if err != nil { + return "", err + } + if env["COMPOSE_FILE"] != "" { + return env["COMPOSE_FILE"], nil + } + } + + return "", nil } -func (r *runner) getEnvFiles() ([]string, error) { +func (r *runner) getEnvFiles() []string { var envFiles []string envFile := path.Join(r.LocalWorkspaceFolder, ".env") envFileStat, err := os.Stat(envFile) if err == nil && envFileStat.Mode().IsRegular() { envFiles = append(envFiles, envFile) } - return envFiles, nil + return envFiles } -func (r *runner) startContainer( - ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, +// resolveComposeServiceImage looks up the named devcontainer service in the +// compose project and determines its original image name, falling back to the +// compose default image when the service does not declare one. +func resolveComposeServiceImage( project *composetypes.Project, composeHelper *compose.ComposeHelper, - composeGlobalArgs []string, - container *config.ContainerDetails, - options UpOptions, -) (*config.ContainerDetails, error) { - service := parsedConfig.Config.Service + service string, +) (composetypes.ServiceConfig, string, error) { composeService, err := project.GetService(service) if err != nil { - return nil, fmt.Errorf( + return composetypes.ServiceConfig{}, "", fmt.Errorf( "service %q configured in devcontainer.json not found in Docker Compose configuration", service, ) @@ -442,894 +603,292 @@ func (r *runner) startContainer( if originalImageName == "" { originalImageName, err = composeHelper.GetDefaultImage(project.Name, service) if err != nil { - return nil, fmt.Errorf("get default image: %w", err) + return composetypes.ServiceConfig{}, "", fmt.Errorf("get default image: %w", err) } } - var didRestoreFromPersistedShare bool - if container != nil { - labels := container.Config.Labels - if labels[ConfigFilesLabel] != "" { - configFiles := strings.Split(labels[ConfigFilesLabel], ",") - - persistedBuildFile := checkForPersistedFile( - configFiles, - FeaturesBuildOverrideFilePrefix, - ) - - persistedStartFile := checkForPersistedFile( - configFiles, - FeaturesStartOverrideFilePrefix, - ) - - if (persistedBuildFile.fileExists || !persistedBuildFile.foundLabel) && - persistedStartFile.fileExists { - didRestoreFromPersistedShare = true - - if persistedBuildFile.fileExists { - composeGlobalArgs = append(composeGlobalArgs, "-f", persistedBuildFile.filePath) - } + return composeService, originalImageName, nil +} - if persistedStartFile.fileExists { - composeGlobalArgs = append(composeGlobalArgs, "-f", persistedStartFile.filePath) - } - } - } +func (r *runner) startContainer( + ctx context.Context, + params *startContainerParams, +) (*config.ContainerDetails, error) { + parsedConfig := params.parsedConfig + project := params.project + composeHelper := params.composeHelper + composeGlobalArgs := params.composeGlobalArgs + container := params.container + options := params.options + + composeService, originalImageName, err := resolveComposeServiceImage( + project, + composeHelper, + parsedConfig.Config.Service, + ) + if err != nil { + return nil, err } - if container == nil || !didRestoreFromPersistedShare { - extendResult, err := r.buildAndExtendDockerCompose( - ctx, - parsedConfig, - substitutionContext, - project, - composeHelper, - &composeService, - composeGlobalArgs, - options.FeatureSecretsFile, - ) - if err != nil { - return nil, fmt.Errorf("build and extend docker-compose: %w", err) - } - - if extendResult.composeBuildFilePath != "" { - composeGlobalArgs = append(composeGlobalArgs, "-f", extendResult.composeBuildFilePath) - } - - currentImageName := extendResult.buildImageName - if currentImageName == "" { - currentImageName = originalImageName - } - - imageDetails, err := r.inspectImage(ctx, currentImageName) - if err != nil { - return nil, fmt.Errorf("inspect image: %w", err) - } - - if options.ExtraDevContainerPath != "" { - if extendResult.imageMetadata == nil { - extendResult.imageMetadata = &config.ImageMetadataConfig{} - } - extraConfig, parseErr := config.ParseDevContainerJSONFile(options.ExtraDevContainerPath) - if parseErr != nil { - return nil, parseErr - } - config.AddConfigToImageMetadata(extraConfig, extendResult.imageMetadata) - } + composeGlobalArgs, didRestoreFromPersistedShare := restorePersistedComposeArgs( + container, + composeGlobalArgs, + ) - mergedConfig, err := config.MergeConfiguration( - parsedConfig.Config, - extendResult.imageMetadata.Config, - ) + if container == nil || !didRestoreFromPersistedShare { + composeGlobalArgs, err = r.buildComposeOverrideArgs(ctx, &composeOverrideParams{ + startParams: params, + composeService: &composeService, + originalImageName: originalImageName, + composeGlobalArgs: composeGlobalArgs, + }) if err != nil { - return nil, fmt.Errorf("merge configuration: %w", err) - } - - if err := config.MergeExtraRemoteEnv( - mergedConfig, options.ExtraDevContainerPath, - ); err != nil { return nil, err } - - additionalLabels := map[string]string{ - metadata.ImageMetadataLabel: extendResult.metadataLabel, - config.UserLabel: imageDetails.Config.User, - } - overrideComposeUpFilePath, err := r.extendedDockerComposeUp( - parsedConfig, - mergedConfig, - composeHelper, - &composeService, - originalImageName, - extendResult.buildImageName, - imageDetails, - additionalLabels, - ) - if err != nil { - return nil, fmt.Errorf("extend docker-compose up: %w", err) - } - - if overrideComposeUpFilePath != "" { - composeGlobalArgs = append(composeGlobalArgs, "-f", overrideComposeUpFilePath) - } } if container != nil && options.Recreate { - log.Debugf("Deleting dev container %s due to --recreate", container.ID) - - if err := r.Driver.StopDevContainer(ctx, r.ID); err != nil { - return nil, fmt.Errorf("stop dev container: %w", err) - } - - if err := r.Driver.DeleteDevContainer(ctx, r.ID); err != nil { - return nil, fmt.Errorf("delete dev container: %w", err) + if err := r.recreateDevContainer(ctx, container); err != nil { + return nil, err } } - upArgs := []string{"--project-name", project.Name} - upArgs = append(upArgs, composeGlobalArgs...) - upArgs = append(upArgs, "up", "-d") - if container != nil { - upArgs = append(upArgs, "--no-recreate") - } - upArgs = r.onlyRunServices(upArgs, parsedConfig) - - // start compose - writer := log.Writer(log.LevelInfo) - defer func() { _ = writer.Close() }() - err = composeHelper.Run(ctx, upArgs, nil, writer, writer) - if err != nil { - return nil, fmt.Errorf("docker-compose run: %w", err) - } - - // TODO wait for started event? - containerDetails, err := composeHelper.FindDevContainer(ctx, project.Name, composeService.Name) - if err != nil { - return nil, fmt.Errorf("find dev container: %w", err) - } - - return containerDetails, nil + return r.composeUpAndFindContainer(ctx, &composeUpRunParams{ + project: project, + composeService: &composeService, + composeHelper: composeHelper, + composeGlobalArgs: composeGlobalArgs, + parsedConfig: parsedConfig, + hasExistingContainer: container != nil, + }) } -// prepareComposeBuildInfo modifies a compose project's devcontainer Dockerfile -// to ensure it can be extended with features. If an Image is specified instead -// of a Build, the metadata from the Image is used to populate the build info. -func (r *runner) prepareComposeBuildInfo( - ctx context.Context, - subCtx *config.SubstitutionContext, - composeService *composetypes.ServiceConfig, - buildTarget string, -) (composeBuildInfo, error) { - var dockerFilePath, dockerfileContents string - var imageBuildInfo *config.ImageBuildInfo - var err error - if composeService.Build != nil { - // Read Dockerfile - if path.IsAbs(composeService.Build.Dockerfile) { - dockerFilePath = composeService.Build.Dockerfile - } else { - dockerFilePath = filepath.Join( - composeService.Build.Context, - composeService.Build.Dockerfile, - ) - } - - originalDockerfile, err := os.ReadFile(dockerFilePath) - if err != nil { - return composeBuildInfo{}, err - } - - // Determine build target. If a multi stage build is used, ensure it is - // valid and modify the Dockerfile if necessary. - originalTarget := composeService.Build.Target - if originalTarget != "" { - buildTarget = originalTarget - } else { - lastStageName, modifiedDockerfile, err := dockerfile.EnsureFinalStageName( - string(originalDockerfile), - config.DockerfileDefaultTarget, - ) - if err != nil { - return composeBuildInfo{}, err - } - - buildTarget = lastStageName - // Override Dockerfile if it was modified, otherwise use the original - if modifiedDockerfile != "" { - dockerfileContents = modifiedDockerfile - } else { - dockerfileContents = string(originalDockerfile) - } - } - imageBuildInfo, err = r.getImageBuildInfoFromDockerfile( - subCtx, - string(originalDockerfile), - mappingToMap(composeService.Build.Args), - originalTarget, - ) - if err != nil { - return composeBuildInfo{}, err - } - } else { - imageBuildInfo, err = r.getImageBuildInfoFromImage(ctx, subCtx, composeService.Image) - if err != nil { - return composeBuildInfo{}, err - } +// restorePersistedComposeArgs detects persisted feature override files recorded +// on an existing container and appends them as additional compose "-f" args. +// It reports whether a usable persisted share was found. +func restorePersistedComposeArgs( + container *config.ContainerDetails, + composeGlobalArgs []string, +) ([]string, bool) { + if container == nil { + return composeGlobalArgs, false } - return composeBuildInfo{ - imageBuildInfo: imageBuildInfo, - dockerfileContents: dockerfileContents, - buildTarget: buildTarget, - }, nil -} -// This extends the build information for docker compose containers. -func (r *runner) buildAndExtendDockerCompose( - ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - project *composetypes.Project, - composeHelper *compose.ComposeHelper, - composeService *composetypes.ServiceConfig, - globalArgs []string, - featureSecretsFile string, -) (composeExtendResult, error) { - var dockerFilePath, dockerfileContents, dockerComposeFilePath string - var imageBuildInfo *config.ImageBuildInfo - var err error - - buildTarget := "dev_container_auto_added_stage_label" - - // Determine base imageName for generated features build - buildInfo, err := r.prepareComposeBuildInfo( - ctx, - substitutionContext, - composeService, - buildTarget, - ) - if err != nil { - return composeExtendResult{}, err - } - imageBuildInfo = buildInfo.imageBuildInfo - dockerfileContents = buildInfo.dockerfileContents - buildTarget = buildInfo.buildTarget - - secretOpts := &feature.SecretOptions{ - SecretsFile: featureSecretsFile, - Prompter: &feature.TerminalSecretPrompter{}, - } - extendImageBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildParams{ - Ctx: substitutionContext, - ImageBuildInfo: imageBuildInfo, - Target: buildTarget, - DevContainerConfig: parsedConfig, - ForceBuild: false, - SecretOpts: secretOpts, - }) - if err != nil { - return composeExtendResult{}, err + labels := container.Config.Labels + if labels[ConfigFilesLabel] == "" { + return composeGlobalArgs, false } - hasFeatures := extendImageBuildInfo != nil && extendImageBuildInfo.FeaturesBuildInfo != nil - buildImageName, err := composeBuildImageName( - composeHelper, - project.Name, - composeService, - hasFeatures, - ) - if err != nil { - return composeExtendResult{}, err - } - - if hasFeatures { - // If the dockerfile is empty (because an Image was used), reference that - // image as the build target after the features / modified contents. - if dockerfileContents == "" { - if composeService.Image == "" && composeService.Build == nil { - return composeExtendResult{}, fmt.Errorf( - "compose service %q has no image or build configuration", - composeService.Name, - ) - } - sanitizedImage := strings.ReplaceAll( - strings.ReplaceAll(composeService.Image, "\n", ""), - "\r", - "", - ) - dockerfileContents = fmt.Sprintf("FROM %s AS %s\n", sanitizedImage, buildTarget) - } - - // Write the final Dockerfile with features - extendedDockerfilePath, extendedDockerfileContent := r.extendedDockerfile( - extendImageBuildInfo.FeaturesBuildInfo, - dockerFilePath, - dockerfileContents, - ) - - log.Debugf( - "Creating extended Dockerfile %s with content: \n %s", - extendedDockerfilePath, - extendedDockerfileContent, - ) + configFiles := strings.Split(labels[ConfigFilesLabel], ",") + persistedBuildFile := checkForPersistedFile(configFiles, FeaturesBuildOverrideFilePrefix) + persistedStartFile := checkForPersistedFile(configFiles, FeaturesStartOverrideFilePrefix) - defer func() { _ = os.RemoveAll(filepath.Dir(extendedDockerfilePath)) }() - - // Write the final docker-compose referencing the modified Dockerfile or Image - dockerComposeFilePath, err = r.extendedDockerComposeBuild( - composeService, - buildImageName, - extendedDockerfilePath, - extendedDockerfileContent, - extendImageBuildInfo.FeaturesBuildInfo, - ) - if err != nil { - return composeExtendResult{buildImageName: buildImageName}, err - } + usablePersistedShare := (persistedBuildFile.fileExists || !persistedBuildFile.foundLabel) && + persistedStartFile.fileExists + if !usablePersistedShare { + return composeGlobalArgs, false } - // Prepare the docker-compose build arguments - buildArgs := []string{"--project-name", project.Name} - buildArgs = append(buildArgs, globalArgs...) - if dockerComposeFilePath != "" { - buildArgs = append(buildArgs, "-f", dockerComposeFilePath) + if persistedBuildFile.fileExists { + composeGlobalArgs = append(composeGlobalArgs, "-f", persistedBuildFile.filePath) } - buildArgs = append(buildArgs, "build") - if extendImageBuildInfo == nil { - buildArgs = append(buildArgs, "--pull") + if persistedStartFile.fileExists { + composeGlobalArgs = append(composeGlobalArgs, "-f", persistedStartFile.filePath) } + return composeGlobalArgs, true +} - // Only run the services defined in .devcontainer.json runServices - if len(parsedConfig.Config.RunServices) > 0 { - buildArgs = append(buildArgs, composeService.Name) - for _, service := range parsedConfig.Config.RunServices { - if service == composeService.Name { - continue - } - buildArgs = append(buildArgs, service) - } - } +// composeOverrideParams groups the inputs for building the feature build/up +// override files and the resulting compose "-f" arguments. +type composeOverrideParams struct { + startParams *startContainerParams + composeService *composetypes.ServiceConfig + originalImageName string + composeGlobalArgs []string +} - // build image - writer := log.Writer(log.LevelInfo) - defer func() { _ = writer.Close() }() - log.Debugf("Run %s %s", composeHelper.Command, strings.Join(buildArgs, " ")) - err = composeHelper.Run(ctx, buildArgs, nil, writer, writer) +// buildComposeOverrideArgs builds and feature-extends the compose project, then +// generates the "up" override, returning composeGlobalArgs extended with any +// generated override files. +func (r *runner) buildComposeOverrideArgs( + ctx context.Context, + params *composeOverrideParams, +) ([]string, error) { + start := params.startParams + composeGlobalArgs := params.composeGlobalArgs + + extendResult, err := r.buildAndExtendDockerCompose(ctx, &buildAndExtendParams{ + parsedConfig: start.parsedConfig, + substitutionContext: start.substitutionContext, + project: start.project, + composeHelper: start.composeHelper, + composeService: params.composeService, + globalArgs: composeGlobalArgs, + featureSecretsFile: start.options.FeatureSecretsFile, + }) if err != nil { - return composeExtendResult{buildImageName: buildImageName}, err + return nil, fmt.Errorf("build and extend docker-compose: %w", err) } - imageMetadata, err := metadata.GetDevContainerMetadata( - substitutionContext, - imageBuildInfo.Metadata, - parsedConfig, - extendImageBuildInfo.Features, - ) - if err != nil { - return composeExtendResult{buildImageName: buildImageName}, err + if extendResult.composeBuildFilePath != "" { + composeGlobalArgs = append(composeGlobalArgs, "-f", extendResult.composeBuildFilePath) } - return composeExtendResult{ - buildImageName: buildImageName, - composeBuildFilePath: dockerComposeFilePath, - imageMetadata: imageMetadata, - metadataLabel: extendImageBuildInfo.MetadataLabel, - }, nil -} - -func (r *runner) extendedDockerfile( - featureBuildInfo *feature.BuildInfo, - dockerfilePath, dockerfileContent string, -) (string, string) { - // extra args? - finalDockerfilePath := dockerfilePath - finalDockerfileContent := dockerfileContent - - // get extended build info - if featureBuildInfo != nil { - // rewrite dockerfile path - finalDockerfilePath = filepath.Join( - featureBuildInfo.FeaturesFolder, - "Dockerfile-with-features", - ) - - // rewrite dockerfile - finalDockerfileContent = dockerfile.RemoveSyntaxVersion(dockerfileContent) - finalDockerfileContent = strings.TrimSpace(strings.Join([]string{ - featureBuildInfo.DockerfilePrefixContent, - strings.TrimSpace(finalDockerfileContent), - featureBuildInfo.DockerfileContent, - }, "\n")) + currentImageName := extendResult.buildImageName + if currentImageName == "" { + currentImageName = params.originalImageName } - return finalDockerfilePath, finalDockerfileContent -} - -func (r *runner) setBuildPathsForContext( - originalContext, dockerFilePath, dockerfileContent, featuresFolder string, -) (relDockerfilePath string, modifiedDockerfileContent string, err error) { - absBuildContext, err := filepath.Abs(originalContext) + imageDetails, err := r.inspectImage(ctx, currentImageName) if err != nil { - return "", "", err + return nil, fmt.Errorf("inspect image: %w", err) } - absDockerFilePath, err := filepath.Abs(dockerFilePath) - if err != nil { - return "", "", err - } - relDockerfilePath, err = filepath.Rel(absBuildContext, absDockerFilePath) + overrideComposeUpFilePath, err := r.generateComposeUpOverride( + params, + extendResult, + imageDetails, + ) if err != nil { - return "", "", err + return nil, err } - absFeatureFolder, err := filepath.Abs(featuresFolder) - if err != nil { - return "", "", err + if overrideComposeUpFilePath != "" { + composeGlobalArgs = append(composeGlobalArgs, "-f", overrideComposeUpFilePath) } - relFeaturePath, err := filepath.Rel(absBuildContext, absFeatureFolder) - if err != nil { - return "", "", err - } - - // Rewrite COPY/ADD directives that reference the features folder to use the relative path - // from the custom build context. This ensures that the features folder is referenced in the - // Dockerfile. - pattern := regexp.MustCompile( - `(COPY|ADD)(\s+)\./` + regexp.QuoteMeta(config.DevsyContextFeatureFolder) + `/`, - ) - modifiedDockerfileContent = pattern.ReplaceAllString( - dockerfileContent, - "${1}${2}./"+filepath.ToSlash(relFeaturePath)+"/", - ) - - return relDockerfilePath, modifiedDockerfileContent, nil -} -type buildContextResult struct { - context string - dockerfilePathInContext string - dockerfileContent string + return composeGlobalArgs, nil } -func (r *runner) extendedDockerComposeBuild( - composeService *composetypes.ServiceConfig, - buildImageName string, - dockerFilePath string, - dockerfileContent string, - featuresBuildInfo *feature.BuildInfo, +// generateComposeUpOverride merges the image metadata into the devcontainer +// config and writes the compose "up" override file, returning its path. +func (r *runner) generateComposeUpOverride( + params *composeOverrideParams, + extendResult composeExtendResult, + imageDetails *config.ImageDetails, ) (string, error) { - result, err := r.prepareBuildContext( - composeService, dockerFilePath, dockerfileContent, featuresBuildInfo, + start := params.startParams + + mergedConfig, err := mergeImageMetadataConfig( + start.parsedConfig, + extendResult.imageMetadata, + start.options.ExtraDevContainerPath, ) if err != nil { return "", err } - if err := os.WriteFile(dockerFilePath, []byte(result.dockerfileContent), 0o600); err != nil { - return "", err + additionalLabels := map[string]string{ + metadata.ImageMetadataLabel: extendResult.metadataLabel, + config.UserLabel: imageDetails.Config.User, } - - service := r.createComposeService( - composeService, - buildImageName, - result.dockerfilePathInContext, - result.context, - featuresBuildInfo, - ) - return r.writeComposeFile(service) -} - -func (r *runner) prepareBuildContext( - composeService *composetypes.ServiceConfig, - dockerFilePath, dockerfileContent string, - featuresBuildInfo *feature.BuildInfo, -) (*buildContextResult, error) { - buildContext := filepath.Dir(featuresBuildInfo.FeaturesFolder) - relDockerFilePath, err := filepath.Rel(buildContext, dockerFilePath) + overrideComposeUpFilePath, err := r.extendedDockerComposeUp(&composeUpParams{ + parsedConfig: start.parsedConfig, + mergedConfig: mergedConfig, + composeHelper: start.composeHelper, + composeService: params.composeService, + originalImageName: params.originalImageName, + overrideImageName: extendResult.buildImageName, + imageDetails: imageDetails, + additionalLabels: additionalLabels, + }) if err != nil { - return nil, err + return "", fmt.Errorf("extend docker-compose up: %w", err) } - result := &buildContextResult{ - context: buildContext, - dockerfilePathInContext: relDockerFilePath, - dockerfileContent: dockerfileContent, - } + return overrideComposeUpFilePath, nil +} - if composeService.Build != nil && composeService.Build.Context != "" { - relDockerFilePath, modifiedDockerfileContent, err := r.setBuildPathsForContext( - composeService.Build.Context, - dockerFilePath, - dockerfileContent, - featuresBuildInfo.FeaturesFolder, - ) +// mergeImageMetadataConfig folds any extra devcontainer config into the image +// metadata, merges it with the parsed config, and applies extra remote env, +// returning the resulting merged devcontainer config. +func mergeImageMetadataConfig( + parsedConfig *config.SubstitutedConfig, + imageMetadata *config.ImageMetadataConfig, + extraDevContainerPath string, +) (*config.MergedDevContainerConfig, error) { + if extraDevContainerPath != "" { + if imageMetadata == nil { + imageMetadata = &config.ImageMetadataConfig{} + } + extraConfig, err := config.ParseDevContainerJSONFile(extraDevContainerPath) if err != nil { return nil, err } - log.Debugf( - "modified Dockerfile path in context to %s and content for extended compose build context %s", - relDockerFilePath, - composeService.Build.Context, - ) - result.context = composeService.Build.Context - result.dockerfilePathInContext = relDockerFilePath - result.dockerfileContent = modifiedDockerfileContent - } - - return result, nil -} - -func (r *runner) createComposeService( - composeService *composetypes.ServiceConfig, - buildImageName string, - dockerfilePathInContext, buildContext string, - featuresBuildInfo *feature.BuildInfo, -) *composetypes.ServiceConfig { - service := &composetypes.ServiceConfig{ - Name: composeService.Name, - Build: &composetypes.BuildConfig{ - Dockerfile: dockerfilePathInContext, - Context: buildContext, - }, - } - if buildImageName != "" { - service.Image = stripDigestFromImageRef(buildImageName) - } - - if composeService.Build != nil && composeService.Build.Target != "" { - service.Build.Target = featuresBuildInfo.OverrideTarget - } - - service.Build.Args = composetypes.NewMappingWithEquals([]string{"BUILDKIT_INLINE_CACHE=1"}) - for k, v := range featuresBuildInfo.BuildArgs { - service.Build.Args[k] = &v - } - - return service -} - -func composeBuildImageName( - composeHelper *compose.ComposeHelper, - projectName string, - composeService *composetypes.ServiceConfig, - hasFeatures bool, -) (string, error) { - if hasFeatures && composeService.Image != "" && composeService.Build == nil { - return composeHelper.GetDefaultImage(projectName, composeService.Name) - } - - if composeService.Image != "" { - return composeService.Image, nil - } - - return composeHelper.GetDefaultImage(projectName, composeService.Name) -} - -func (r *runner) writeComposeFile(service *composetypes.ServiceConfig) (string, error) { - project := &composetypes.Project{ - Services: map[string]composetypes.ServiceConfig{ - service.Name: *service, - }, - } - - dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) - if err := os.MkdirAll(dockerComposeFolder, 0o750); err != nil { - return "", err + config.AddConfigToImageMetadata(extraConfig, imageMetadata) } - dockerComposeData, err := yaml.Marshal(project) + mergedConfig, err := config.MergeConfiguration(parsedConfig.Config, imageMetadata.Config) if err != nil { - return "", err + return nil, fmt.Errorf("merge configuration: %w", err) } - dockerComposePath := filepath.Join( - dockerComposeFolder, - fmt.Sprintf("%s-%d.yml", FeaturesBuildOverrideFilePrefix, time.Now().Second()), - ) - - log.Debugf( - "Creating docker-compose build %s with content:\n %s", - dockerComposePath, - string(dockerComposeData), - ) - - if err := os.WriteFile(dockerComposePath, dockerComposeData, 0o600); err != nil { - return "", err - } - - return dockerComposePath, nil -} - -func stripDigestFromImageRef(imageRef string) string { - baseRef, _, found := strings.Cut(imageRef, "@") - if !found { - return imageRef + if err := config.MergeExtraRemoteEnv(mergedConfig, extraDevContainerPath); err != nil { + return nil, err } - return baseRef -} - -// mountToServiceVolumeConfig forwards Mount.Other options into the -// override Compose volume so they survive override generation; without -// this, bind/volume/tmpfs options are silently dropped. -func mountToServiceVolumeConfig(m *config.Mount) composetypes.ServiceVolumeConfig { - v := composetypes.ServiceVolumeConfig{ - Type: m.Type, - Source: m.Source, - Target: m.Target, - ReadOnly: m.IsReadOnly(), - Consistency: m.Consistency(), - } - switch m.Type { - case composetypes.VolumeTypeBind, "": - v.Bind = bindOptionsFromMount(m) - case composetypes.VolumeTypeVolume: - v.Volume = volumeOptionsFromMount(m) - case composetypes.VolumeTypeTmpfs: - v.Tmpfs = tmpfsOptionsFromMount(m) - } - return v -} - -func bindOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeBind { - propagation := m.BindPropagation() - nonRecursive := m.IsBindNonRecursive() - if propagation == "" && !nonRecursive { - return nil - } - bind := &composetypes.ServiceVolumeBind{Propagation: propagation} - if nonRecursive { - bind.Recursive = "disabled" - } - return bind + return mergedConfig, nil } -func volumeOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeVolume { - nocopy := m.VolumeNoCopy() - subpath := m.VolumeSubpath() - if !nocopy && subpath == "" { - return nil - } - return &composetypes.ServiceVolumeVolume{NoCopy: nocopy, Subpath: subpath} -} +// recreateDevContainer stops and deletes an existing dev container so it can be +// recreated, in response to the --recreate option. +func (r *runner) recreateDevContainer( + ctx context.Context, + container *config.ContainerDetails, +) error { + log.Debugf("Deleting dev container %s due to --recreate", container.ID) -func tmpfsOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeTmpfs { - size, hasSize := parseTmpfsSize(m.TmpfsSize(), m.Target) - mode, hasMode := parseTmpfsMode(m.TmpfsMode(), m.Target) - if !hasSize && !hasMode { - return nil + if err := r.Driver.StopDevContainer(ctx, r.ID); err != nil { + return fmt.Errorf("stop dev container: %w", err) } - return &composetypes.ServiceVolumeTmpfs{Size: size, Mode: mode} -} -func parseTmpfsSize(raw, target string) (composetypes.UnitBytes, bool) { - if raw == "" { - return 0, false - } - parsed, err := strconv.ParseInt(raw, 10, 64) - if err != nil { - log.Warnf("ignoring tmpfs-size %q on mount %s: %s", raw, target, err) - return 0, false + if err := r.Driver.DeleteDevContainer(ctx, r.ID); err != nil { + return fmt.Errorf("delete dev container: %w", err) } - return composetypes.UnitBytes(parsed), true + return nil } -func parseTmpfsMode(raw, target string) (uint32, bool) { - if raw == "" { - return 0, false - } - parsed, err := strconv.ParseUint(raw, 8, 32) - if err != nil { - log.Warnf("ignoring tmpfs-mode %q on mount %s: %s", raw, target, err) - return 0, false - } - return uint32(parsed), true +// composeUpRunParams groups the inputs for the final "docker compose up" step. +type composeUpRunParams struct { + project *composetypes.Project + composeService *composetypes.ServiceConfig + composeHelper *compose.ComposeHelper + composeGlobalArgs []string + parsedConfig *config.SubstitutedConfig + hasExistingContainer bool } -func (r *runner) extendedDockerComposeUp( - parsedConfig *config.SubstitutedConfig, - mergedConfig *config.MergedDevContainerConfig, - composeHelper *compose.ComposeHelper, - composeService *composetypes.ServiceConfig, - originalImageName, - overrideImageName string, - imageDetails *config.ImageDetails, - additionalLabels map[string]string, -) (string, error) { - dockerComposeUpProject := r.generateDockerComposeUpProject( - parsedConfig, - mergedConfig, - composeHelper, - composeService, - originalImageName, - overrideImageName, - imageDetails, - additionalLabels, - ) - dockerComposeData, err := yaml.Marshal(dockerComposeUpProject) - if err != nil { - return "", err +// composeUpAndFindContainer runs "docker compose up -d" with the assembled +// arguments and returns the resulting dev container details. +func (r *runner) composeUpAndFindContainer( + ctx context.Context, + params *composeUpRunParams, +) (*config.ContainerDetails, error) { + upArgs := []string{composeProjectNameFlag, params.project.Name} + upArgs = append(upArgs, params.composeGlobalArgs...) + upArgs = append(upArgs, "up", "-d") + if params.hasExistingContainer { + upArgs = append(upArgs, "--no-recreate") } + upArgs = r.onlyRunServices(upArgs, params.parsedConfig) - dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) - err = os.MkdirAll(dockerComposeFolder, 0o750) - if err != nil { - return "", err + writer := log.Writer(log.LevelInfo) + defer func() { _ = writer.Close() }() + if err := params.composeHelper.Run(ctx, upArgs, nil, writer, writer); err != nil { + return nil, fmt.Errorf("docker-compose run: %w", err) } - dockerComposePath := filepath.Join( - dockerComposeFolder, - fmt.Sprintf("%s-%d.yml", FeaturesStartOverrideFilePrefix, time.Now().Second()), - ) - - log.Debugf( - "Creating docker-compose up %s with content:\n %s", - dockerComposePath, - string(dockerComposeData), + // TODO wait for started event? + containerDetails, err := params.composeHelper.FindDevContainer( + ctx, + params.project.Name, + params.composeService.Name, ) - - err = os.WriteFile(dockerComposePath, dockerComposeData, 0o600) if err != nil { - return "", err - } - return dockerComposePath, nil -} - -func (r *runner) generateDockerComposeUpProject( - parsedConfig *config.SubstitutedConfig, - mergedConfig *config.MergedDevContainerConfig, - composeHelper *compose.ComposeHelper, - composeService *composetypes.ServiceConfig, - originalImageName, - overrideImageName string, - imageDetails *config.ImageDetails, - additionalLabels map[string]string, -) *composetypes.Project { - // Configure overridden service - userEntrypoint := composeService.Entrypoint - userCommand := composeService.Command - if mergedConfig.OverrideCommand != nil && *mergedConfig.OverrideCommand { - userEntrypoint = []string{} - userCommand = []string{} - } else { - if len(userEntrypoint) == 0 { - userEntrypoint = imageDetails.Config.Entrypoint - } - - if len(userCommand) == 0 { - userCommand = imageDetails.Config.Cmd - } - } - - entrypoint := composetypes.ShellCommand{ - "/bin/sh", - "-c", - `echo Container started -trap "exit 0" 15 -` + strings.Join(mergedConfig.Entrypoints, "\n") + ` -exec "$$@" -` + DefaultEntrypoint, - "-", - } - entrypoint = append(entrypoint, userEntrypoint...) - - labels := composetypes.Labels{} - if len(r.IDLabels) > 0 { - for _, l := range r.IDLabels { - k, v, _ := strings.Cut(l, "=") - v = regexp.MustCompile(`\$`).ReplaceAllString(v, "$$$$") - v = regexp.MustCompile(`'`).ReplaceAllString(v, `\'\'`) - labels[k] = v - } - } else { - labels[config.DockerIDLabel] = r.ID - } - for k, v := range additionalLabels { - // Escape $ and ' to prevent substituting local environment variables! - label := regexp.MustCompile(`\$`).ReplaceAllString(v, "$$$$") - label = regexp.MustCompile(`'`).ReplaceAllString(label, `\'\'`) - labels.Add(k, label) - } - - overrideService := &composetypes.ServiceConfig{ - Name: composeService.Name, - Entrypoint: entrypoint, - Environment: mappingFromMap(mergedConfig.ContainerEnv), - Init: mergedConfig.Init, - CapAdd: mergedConfig.CapAdd, - SecurityOpt: mergedConfig.SecurityOpt, - Labels: labels, - } - - if originalImageName != overrideImageName { - overrideService.Image = overrideImageName - } - - if !reflect.DeepEqual(userCommand, composeService.Command) { - overrideService.Command = userCommand - } - - if mergedConfig.ContainerUser != "" { - overrideService.User = mergedConfig.ContainerUser - } - - if mergedConfig.Privileged != nil { - overrideService.Privileged = *mergedConfig.Privileged - } - - gpuSupportEnabled := r.resolveComposeGPUAvailability(composeHelper) - r.configureGPUResources(parsedConfig, gpuSupportEnabled, overrideService) - - for _, mount := range mergedConfig.Mounts { - overrideService.Volumes = append( - overrideService.Volumes, - mountToServiceVolumeConfig(mount), - ) - } - - project := &composetypes.Project{} - project.Services = map[string]composetypes.ServiceConfig{ - overrideService.Name: *overrideService, - } - - // Configure volumes - var volumeMounts []composetypes.VolumeConfig - for _, m := range mergedConfig.Mounts { - if m.Type == "volume" { - volumeMounts = append(volumeMounts, composetypes.VolumeConfig{ - Name: m.Source, - External: composetypes.External(m.External), - }) - } - } - - if len(volumeMounts) > 0 { - project.Volumes = map[string]composetypes.VolumeConfig{} - } - for _, volumeMount := range volumeMounts { - project.Volumes[volumeMount.Name] = volumeMount - } - - return project -} - -func (r *runner) resolveComposeGPUAvailability(composeHelper *compose.ComposeHelper) bool { - switch r.WorkspaceConfig.CLIOptions.GPUAvailability { - case stringTrue: - return true - case stringFalse: - return false - default: - available, _ := composeHelper.Docker.GPUSupportEnabled() - return available + return nil, fmt.Errorf("find dev container: %w", err) } -} -func (r *runner) configureGPUResources( - parsedConfig *config.SubstitutedConfig, - gpuSupportEnabled bool, - overrideService *composetypes.ServiceConfig, -) { - if parsedConfig.Config.HostRequirements != nil { - enableGPU, warnIfMissing := parsedConfig.Config.HostRequirements.ShouldEnableGPU( - gpuSupportEnabled, - ) - if enableGPU { - overrideService.Deploy = &composetypes.DeployConfig{ - Resources: composetypes.Resources{ - Reservations: &composetypes.Resource{ - Devices: []composetypes.DeviceRequest{ - { - Capabilities: []string{"gpu"}, - }, - }, - }, - }, - } - } - if warnIfMissing { - log.Warn("GPU required but not available on host") - } - } + return containerDetails, nil } func checkForPersistedFile( diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go new file mode 100644 index 000000000..4c870c7ff --- /dev/null +++ b/pkg/devcontainer/compose_build.go @@ -0,0 +1,615 @@ +package devcontainer + +import ( + "context" + "fmt" + "os" + "path" + "path/filepath" + "regexp" + "strings" + "time" + + composetypes "github.com/compose-spec/compose-go/v2/types" + "github.com/devsy-org/devsy/pkg/compose" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/devcontainer/feature" + "github.com/devsy-org/devsy/pkg/devcontainer/metadata" + "github.com/devsy-org/devsy/pkg/dockerfile" + "github.com/devsy-org/devsy/pkg/log" + "gopkg.in/yaml.v3" +) + +// featureFolderCopyPattern matches COPY/ADD directives that reference the +// devsy feature folder so they can be rewritten to a path relative to a custom +// build context. Compiled once because the folder name is a build-time constant. +var featureFolderCopyPattern = regexp.MustCompile( + `(COPY|ADD)(\s+)\./` + regexp.QuoteMeta(config.DevsyContextFeatureFolder) + `/`, +) + +// prepareComposeBuildInfo modifies a compose project's devcontainer Dockerfile +// to ensure it can be extended with features. If an Image is specified instead +// of a Build, the metadata from the Image is used to populate the build info. +func (r *runner) prepareComposeBuildInfo( + ctx context.Context, + subCtx *config.SubstitutionContext, + composeService *composetypes.ServiceConfig, + buildTarget string, +) (composeBuildInfo, error) { + if composeService.Build == nil { + imageBuildInfo, err := r.getImageBuildInfoFromImage(ctx, subCtx, composeService.Image) + if err != nil { + return composeBuildInfo{}, err + } + return composeBuildInfo{imageBuildInfo: imageBuildInfo, buildTarget: buildTarget}, nil + } + + return r.prepareComposeDockerfileBuildInfo(subCtx, composeService) +} + +// prepareComposeDockerfileBuildInfo handles the Build-backed branch of +// prepareComposeBuildInfo: it reads the service Dockerfile, resolves the build +// target (ensuring a final stage name for multi-stage builds), and extracts the +// image build info. +func (r *runner) prepareComposeDockerfileBuildInfo( + subCtx *config.SubstitutionContext, + composeService *composetypes.ServiceConfig, +) (composeBuildInfo, error) { + dockerFilePath := composeService.Build.Dockerfile + if !path.IsAbs(dockerFilePath) { + dockerFilePath = filepath.Join(composeService.Build.Context, dockerFilePath) + } + + // #nosec G304 -- dockerFilePath is derived from trusted devcontainer config. + originalDockerfile, err := os.ReadFile(dockerFilePath) + if err != nil { + return composeBuildInfo{}, err + } + + // Determine build target. If a multi stage build is used, ensure it is + // valid and modify the Dockerfile if necessary. + originalTarget := composeService.Build.Target + var dockerfileContents string + var buildTarget string + if originalTarget != "" { + buildTarget = originalTarget + } else { + lastStageName, modifiedDockerfile, stageErr := dockerfile.EnsureFinalStageName( + string(originalDockerfile), + config.DockerfileDefaultTarget, + ) + if stageErr != nil { + return composeBuildInfo{}, stageErr + } + + buildTarget = lastStageName + // Override Dockerfile if it was modified, otherwise use the original + if modifiedDockerfile != "" { + dockerfileContents = modifiedDockerfile + } else { + dockerfileContents = string(originalDockerfile) + } + } + + imageBuildInfo, err := r.getImageBuildInfoFromDockerfile( + subCtx, + string(originalDockerfile), + mappingToMap(composeService.Build.Args), + originalTarget, + ) + if err != nil { + return composeBuildInfo{}, err + } + + return composeBuildInfo{ + imageBuildInfo: imageBuildInfo, + dockerfileContents: dockerfileContents, + buildTarget: buildTarget, + }, nil +} + +// This extends the build information for docker compose containers. +func (r *runner) buildAndExtendDockerCompose( + ctx context.Context, + params *buildAndExtendParams, +) (composeExtendResult, error) { + prepared, err := r.prepareExtendedComposeBuild(ctx, params) + if err != nil { + return composeExtendResult{}, err + } + extendImageBuildInfo := prepared.extendImageBuildInfo + + buildImageName, err := composeBuildImageName( + params.composeHelper, + params.project.Name, + params.composeService, + hasFeatureBuildInfo(extendImageBuildInfo), + ) + if err != nil { + return composeExtendResult{}, err + } + + dockerComposeFilePath, cleanup, err := r.composeFeatureOverride( + prepared, + params.composeService, + buildImageName, + ) + defer cleanup() + if err != nil { + return composeExtendResult{buildImageName: buildImageName}, err + } + + buildArgs := composeBuildArgs(&composeBuildArgsParams{ + projectName: params.project.Name, + globalArgs: params.globalArgs, + overrideComposeFilePath: dockerComposeFilePath, + pull: extendImageBuildInfo == nil, + serviceName: params.composeService.Name, + runServices: params.parsedConfig.Config.RunServices, + }) + + if err := r.runComposeBuild(ctx, params.composeHelper, buildArgs); err != nil { + return composeExtendResult{buildImageName: buildImageName}, err + } + + imageMetadata, err := metadata.GetDevContainerMetadata( + params.substitutionContext, + prepared.imageBuildInfo.Metadata, + params.parsedConfig, + extendImageBuildInfo.Features, + ) + if err != nil { + return composeExtendResult{buildImageName: buildImageName}, err + } + + return composeExtendResult{ + buildImageName: buildImageName, + composeBuildFilePath: dockerComposeFilePath, + imageMetadata: imageMetadata, + metadataLabel: extendImageBuildInfo.MetadataLabel, + }, nil +} + +// composeFeatureOverride builds the feature override compose file when the build +// has features, returning the override path (empty when none) and a cleanup +// function that is always safe to defer. +func (r *runner) composeFeatureOverride( + prepared preparedComposeBuild, + composeService *composetypes.ServiceConfig, + buildImageName string, +) (string, func(), error) { + if !hasFeatureBuildInfo(prepared.extendImageBuildInfo) { + return "", func() {}, nil + } + return r.writeFeatureBuildOverride(&featureBuildOverrideParams{ + extendImageBuildInfo: prepared.extendImageBuildInfo, + composeService: composeService, + buildImageName: buildImageName, + dockerfileContents: prepared.dockerfileContents, + buildTarget: prepared.buildTarget, + }) +} + +// preparedComposeBuild holds the resolved base build info and feature-extended +// build info used to assemble the compose build. +type preparedComposeBuild struct { + imageBuildInfo *config.ImageBuildInfo + extendImageBuildInfo *feature.ExtendedBuildInfo + dockerfileContents string + buildTarget string +} + +// prepareExtendedComposeBuild resolves the base image build info for the compose +// service and computes the feature-extended build info on top of it. +func (r *runner) prepareExtendedComposeBuild( + ctx context.Context, + params *buildAndExtendParams, +) (preparedComposeBuild, error) { + const defaultBuildTarget = "dev_container_auto_added_stage_label" + + buildInfo, err := r.prepareComposeBuildInfo( + ctx, + params.substitutionContext, + params.composeService, + defaultBuildTarget, + ) + if err != nil { + return preparedComposeBuild{}, err + } + + extendImageBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildParams{ + Ctx: params.substitutionContext, + ImageBuildInfo: buildInfo.imageBuildInfo, + Target: buildInfo.buildTarget, + DevContainerConfig: params.parsedConfig, + ForceBuild: false, + SecretOpts: &feature.SecretOptions{ + SecretsFile: params.featureSecretsFile, + Prompter: &feature.TerminalSecretPrompter{}, + }, + }) + if err != nil { + return preparedComposeBuild{}, err + } + + return preparedComposeBuild{ + imageBuildInfo: buildInfo.imageBuildInfo, + extendImageBuildInfo: extendImageBuildInfo, + dockerfileContents: buildInfo.dockerfileContents, + buildTarget: buildInfo.buildTarget, + }, nil +} + +// hasFeatureBuildInfo reports whether the extended build info carries a feature +// build that requires generating an override Dockerfile and compose file. +func hasFeatureBuildInfo(extendImageBuildInfo *feature.ExtendedBuildInfo) bool { + return extendImageBuildInfo != nil && extendImageBuildInfo.FeaturesBuildInfo != nil +} + +// runComposeBuild runs "docker compose ... build" with the given arguments, +// streaming output to the info log. +func (r *runner) runComposeBuild( + ctx context.Context, + composeHelper *compose.ComposeHelper, + buildArgs []string, +) error { + writer := log.Writer(log.LevelInfo) + defer func() { _ = writer.Close() }() + log.Debugf("Run %s %s", composeHelper.Command, strings.Join(buildArgs, " ")) + if err := composeHelper.Run(ctx, buildArgs, nil, writer, writer); err != nil { + return err + } + return nil +} + +// featureBuildOverrideParams groups the inputs for writing the extended +// Dockerfile and compose build override used when features are present. +type featureBuildOverrideParams struct { + extendImageBuildInfo *feature.ExtendedBuildInfo + composeService *composetypes.ServiceConfig + buildImageName string + dockerfileContents string + buildTarget string +} + +// writeFeatureBuildOverride writes the extended Dockerfile and the compose build +// override file referencing it. It returns the override compose file path and a +// cleanup function that removes the temporary extended Dockerfile directory. The +// cleanup function is always non-nil and safe to defer even on error. +func (r *runner) writeFeatureBuildOverride( + params *featureBuildOverrideParams, +) (composeFilePath string, cleanup func(), err error) { + cleanup = func() {} + + dockerfileContents := params.dockerfileContents + // If the dockerfile is empty (because an Image was used), reference that + // image as the build target after the features / modified contents. + if dockerfileContents == "" { + if params.composeService.Image == "" && params.composeService.Build == nil { + return "", cleanup, fmt.Errorf( + "compose service %q has no image or build configuration", + params.composeService.Name, + ) + } + sanitizedImage := strings.ReplaceAll( + strings.ReplaceAll(params.composeService.Image, "\n", ""), + "\r", + "", + ) + dockerfileContents = fmt.Sprintf("FROM %s AS %s\n", sanitizedImage, params.buildTarget) + } + + // The feature build info rewrites the Dockerfile path, so the original path + // is intentionally empty here. + extendedDockerfilePath, extendedDockerfileContent := r.extendedDockerfile( + params.extendImageBuildInfo.FeaturesBuildInfo, + "", + dockerfileContents, + ) + + log.Debugf( + "Creating extended Dockerfile %s with content: \n %s", + extendedDockerfilePath, + extendedDockerfileContent, + ) + + cleanup = func() { _ = os.RemoveAll(filepath.Dir(extendedDockerfilePath)) } + + composeFilePath, err = r.extendedDockerComposeBuild(&extendedComposeBuildParams{ + composeService: params.composeService, + buildImageName: params.buildImageName, + dockerFilePath: extendedDockerfilePath, + dockerfileContent: extendedDockerfileContent, + featuresBuildInfo: params.extendImageBuildInfo.FeaturesBuildInfo, + }) + if err != nil { + return "", cleanup, err + } + + return composeFilePath, cleanup, nil +} + +// composeBuildArgsParams groups the inputs for assembling compose build args. +type composeBuildArgsParams struct { + projectName string + globalArgs []string + overrideComposeFilePath string + pull bool + serviceName string + runServices []string +} + +// composeBuildArgs assembles the "docker compose ... build" argument list, +// adding the override file, --pull when no feature build info is present, and +// any explicitly requested run services. +func composeBuildArgs(params *composeBuildArgsParams) []string { + buildArgs := []string{composeProjectNameFlag, params.projectName} + buildArgs = append(buildArgs, params.globalArgs...) + if params.overrideComposeFilePath != "" { + buildArgs = append(buildArgs, "-f", params.overrideComposeFilePath) + } + buildArgs = append(buildArgs, "build") + if params.pull { + buildArgs = append(buildArgs, "--pull") + } + + // Only run the services defined in .devcontainer.json runServices + if len(params.runServices) > 0 { + buildArgs = append(buildArgs, params.serviceName) + for _, service := range params.runServices { + if service == params.serviceName { + continue + } + buildArgs = append(buildArgs, service) + } + } + return buildArgs +} + +func (r *runner) extendedDockerfile( + featureBuildInfo *feature.BuildInfo, + dockerfilePath, dockerfileContent string, +) (string, string) { + // extra args? + finalDockerfilePath := dockerfilePath + finalDockerfileContent := dockerfileContent + + // get extended build info + if featureBuildInfo != nil { + // rewrite dockerfile path + finalDockerfilePath = filepath.Join( + featureBuildInfo.FeaturesFolder, + "Dockerfile-with-features", + ) + + // rewrite dockerfile + finalDockerfileContent = dockerfile.RemoveSyntaxVersion(dockerfileContent) + finalDockerfileContent = strings.TrimSpace(strings.Join([]string{ + featureBuildInfo.DockerfilePrefixContent, + strings.TrimSpace(finalDockerfileContent), + featureBuildInfo.DockerfileContent, + }, "\n")) + } + + return finalDockerfilePath, finalDockerfileContent +} + +func (r *runner) setBuildPathsForContext( + originalContext, dockerFilePath, dockerfileContent, featuresFolder string, +) (relDockerfilePath string, modifiedDockerfileContent string, err error) { + absBuildContext, err := filepath.Abs(originalContext) + if err != nil { + return "", "", err + } + + absDockerFilePath, err := filepath.Abs(dockerFilePath) + if err != nil { + return "", "", err + } + relDockerfilePath, err = filepath.Rel(absBuildContext, absDockerFilePath) + if err != nil { + return "", "", err + } + + absFeatureFolder, err := filepath.Abs(featuresFolder) + if err != nil { + return "", "", err + } + relFeaturePath, err := filepath.Rel(absBuildContext, absFeatureFolder) + if err != nil { + return "", "", err + } + + // Rewrite COPY/ADD directives that reference the features folder to use the relative path + // from the custom build context. This ensures that the features folder is referenced in the + // Dockerfile. + modifiedDockerfileContent = featureFolderCopyPattern.ReplaceAllString( + dockerfileContent, + "${1}${2}./"+filepath.ToSlash(relFeaturePath)+"/", + ) + + return relDockerfilePath, modifiedDockerfileContent, nil +} + +type buildContextResult struct { + context string + dockerfilePathInContext string + dockerfileContent string +} + +// extendedComposeBuildParams groups the inputs for writing the extended compose +// build file referencing a feature-augmented Dockerfile. +type extendedComposeBuildParams struct { + composeService *composetypes.ServiceConfig + buildImageName string + dockerFilePath string + dockerfileContent string + featuresBuildInfo *feature.BuildInfo +} + +func (r *runner) extendedDockerComposeBuild(params *extendedComposeBuildParams) (string, error) { + result, err := r.prepareBuildContext( + params.composeService, + params.dockerFilePath, + params.dockerfileContent, + params.featuresBuildInfo, + ) + if err != nil { + return "", err + } + + if err := os.WriteFile( + params.dockerFilePath, + []byte(result.dockerfileContent), + 0o600, + ); err != nil { + return "", err + } + + service := r.createComposeService(&composeServiceParams{ + composeService: params.composeService, + buildImageName: params.buildImageName, + dockerfilePathInContext: result.dockerfilePathInContext, + buildContext: result.context, + featuresBuildInfo: params.featuresBuildInfo, + }) + return r.writeComposeFile(service) +} + +func (r *runner) prepareBuildContext( + composeService *composetypes.ServiceConfig, + dockerFilePath, dockerfileContent string, + featuresBuildInfo *feature.BuildInfo, +) (*buildContextResult, error) { + buildContext := filepath.Dir(featuresBuildInfo.FeaturesFolder) + relDockerFilePath, err := filepath.Rel(buildContext, dockerFilePath) + if err != nil { + return nil, err + } + + result := &buildContextResult{ + context: buildContext, + dockerfilePathInContext: relDockerFilePath, + dockerfileContent: dockerfileContent, + } + + if composeService.Build != nil && composeService.Build.Context != "" { + relDockerFilePath, modifiedDockerfileContent, err := r.setBuildPathsForContext( + composeService.Build.Context, + dockerFilePath, + dockerfileContent, + featuresBuildInfo.FeaturesFolder, + ) + if err != nil { + return nil, err + } + log.Debugf( + "modified Dockerfile path in context to %s and content for extended compose build context %s", + relDockerFilePath, + composeService.Build.Context, + ) + result.context = composeService.Build.Context + result.dockerfilePathInContext = relDockerFilePath + result.dockerfileContent = modifiedDockerfileContent + } + + return result, nil +} + +// composeServiceParams groups the inputs for building the override compose +// service definition. +type composeServiceParams struct { + composeService *composetypes.ServiceConfig + buildImageName string + dockerfilePathInContext string + buildContext string + featuresBuildInfo *feature.BuildInfo +} + +func (r *runner) createComposeService(params *composeServiceParams) *composetypes.ServiceConfig { + composeService := params.composeService + featuresBuildInfo := params.featuresBuildInfo + + service := &composetypes.ServiceConfig{ + Name: composeService.Name, + Build: &composetypes.BuildConfig{ + Dockerfile: params.dockerfilePathInContext, + Context: params.buildContext, + }, + } + if params.buildImageName != "" { + service.Image = stripDigestFromImageRef(params.buildImageName) + } + + if composeService.Build != nil && composeService.Build.Target != "" { + service.Build.Target = featuresBuildInfo.OverrideTarget + } + + service.Build.Args = composetypes.NewMappingWithEquals([]string{"BUILDKIT_INLINE_CACHE=1"}) + for k, v := range featuresBuildInfo.BuildArgs { + service.Build.Args[k] = &v + } + + return service +} + +func composeBuildImageName( + composeHelper *compose.ComposeHelper, + projectName string, + composeService *composetypes.ServiceConfig, + hasFeatures bool, +) (string, error) { + if hasFeatures && composeService.Image != "" && composeService.Build == nil { + return composeHelper.GetDefaultImage(projectName, composeService.Name) + } + + if composeService.Image != "" { + return composeService.Image, nil + } + + return composeHelper.GetDefaultImage(projectName, composeService.Name) +} + +func (r *runner) writeComposeFile(service *composetypes.ServiceConfig) (string, error) { + project := &composetypes.Project{ + Services: map[string]composetypes.ServiceConfig{ + service.Name: *service, + }, + } + + dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) + if err := os.MkdirAll(dockerComposeFolder, 0o750); err != nil { + return "", err + } + + dockerComposeData, err := yaml.Marshal(project) + if err != nil { + return "", err + } + + dockerComposePath := filepath.Join( + dockerComposeFolder, + fmt.Sprintf("%s-%d.yml", FeaturesBuildOverrideFilePrefix, time.Now().Second()), + ) + + log.Debugf( + "Creating docker-compose build %s with content:\n %s", + dockerComposePath, + string(dockerComposeData), + ) + + if err := os.WriteFile(dockerComposePath, dockerComposeData, 0o600); err != nil { + return "", err + } + + return dockerComposePath, nil +} + +func stripDigestFromImageRef(imageRef string) string { + baseRef, _, found := strings.Cut(imageRef, "@") + if !found { + return imageRef + } + + return baseRef +} diff --git a/pkg/devcontainer/compose_mounts.go b/pkg/devcontainer/compose_mounts.go new file mode 100644 index 000000000..dfb6d29bd --- /dev/null +++ b/pkg/devcontainer/compose_mounts.go @@ -0,0 +1,86 @@ +package devcontainer + +import ( + "strconv" + + composetypes "github.com/compose-spec/compose-go/v2/types" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" +) + +// mountToServiceVolumeConfig forwards Mount.Other options into the +// override Compose volume so they survive override generation; without +// this, bind/volume/tmpfs options are silently dropped. +func mountToServiceVolumeConfig(m *config.Mount) composetypes.ServiceVolumeConfig { + v := composetypes.ServiceVolumeConfig{ + Type: m.Type, + Source: m.Source, + Target: m.Target, + ReadOnly: m.IsReadOnly(), + Consistency: m.Consistency(), + } + switch m.Type { + case composetypes.VolumeTypeBind, "": + v.Bind = bindOptionsFromMount(m) + case composetypes.VolumeTypeVolume: + v.Volume = volumeOptionsFromMount(m) + case composetypes.VolumeTypeTmpfs: + v.Tmpfs = tmpfsOptionsFromMount(m) + } + return v +} + +func bindOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeBind { + propagation := m.BindPropagation() + nonRecursive := m.IsBindNonRecursive() + if propagation == "" && !nonRecursive { + return nil + } + bind := &composetypes.ServiceVolumeBind{Propagation: propagation} + if nonRecursive { + bind.Recursive = "disabled" + } + return bind +} + +func volumeOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeVolume { + nocopy := m.VolumeNoCopy() + subpath := m.VolumeSubpath() + if !nocopy && subpath == "" { + return nil + } + return &composetypes.ServiceVolumeVolume{NoCopy: nocopy, Subpath: subpath} +} + +func tmpfsOptionsFromMount(m *config.Mount) *composetypes.ServiceVolumeTmpfs { + size, hasSize := parseTmpfsSize(m.TmpfsSize(), m.Target) + mode, hasMode := parseTmpfsMode(m.TmpfsMode(), m.Target) + if !hasSize && !hasMode { + return nil + } + return &composetypes.ServiceVolumeTmpfs{Size: size, Mode: mode} +} + +func parseTmpfsSize(raw, target string) (composetypes.UnitBytes, bool) { + if raw == "" { + return 0, false + } + parsed, err := strconv.ParseInt(raw, 10, 64) + if err != nil { + log.Warnf("ignoring tmpfs-size %q on mount %s: %s", raw, target, err) + return 0, false + } + return composetypes.UnitBytes(parsed), true +} + +func parseTmpfsMode(raw, target string) (uint32, bool) { + if raw == "" { + return 0, false + } + parsed, err := strconv.ParseUint(raw, 8, 32) + if err != nil { + log.Warnf("ignoring tmpfs-mode %q on mount %s: %s", raw, target, err) + return 0, false + } + return uint32(parsed), true +} diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 66176f6bd..7ddee0154 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -145,22 +145,22 @@ func (s *ComposeSuite) TestComposeBuildImageName() { func (s *ComposeSuite) TestCreateComposeServiceUsesBuildImageName() { r := &runner{} - service := r.createComposeService( - &composetypes.ServiceConfig{ + service := r.createComposeService(&composeServiceParams{ + composeService: &composetypes.ServiceConfig{ Name: "app", Image: "ghcr.io/example/shared-base:latest", Build: &composetypes.BuildConfig{Target: "original-target"}, }, - "workspace-app:latest", - "Dockerfile-with-features", - "/tmp/context", - &feature.BuildInfo{ + buildImageName: "workspace-app:latest", + dockerfilePathInContext: "Dockerfile-with-features", + buildContext: "/tmp/context", + featuresBuildInfo: &feature.BuildInfo{ OverrideTarget: "dev_containers_target_stage", BuildArgs: map[string]string{ "FEATURE_FLAG": "true", }, }, - ) + }) s.Equal("workspace-app:latest", service.Image) s.Require().NotNil(service.Build) @@ -470,3 +470,106 @@ func TestTmpfsOptionsFromMount(t *testing.T) { }) } } + +func TestEscapeComposeLabelValue(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {name: "plain value untouched", in: "plain-value", want: "plain-value"}, + {name: "dollar doubled", in: "$HOME", want: "$$HOME"}, + {name: "single quote escaped", in: "it's", want: `it\'\'s`}, + {name: "dollar and quote combined", in: "$a'b", want: `$$a\'\'b`}, + {name: "empty value", in: "", want: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := escapeComposeLabelValue(tt.in); got != tt.want { + t.Errorf("escapeComposeLabelValue(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestBuildServiceLabels(t *testing.T) { + t.Run("uses default ID label when no ID labels", func(t *testing.T) { + r := &runner{} + r.ID = "workspace-id" + + labels := r.buildServiceLabels(nil) + + if labels[config.DockerIDLabel] != "workspace-id" { + t.Errorf("default ID label = %q, want %q", labels[config.DockerIDLabel], "workspace-id") + } + }) + + t.Run("escapes ID and additional label values", func(t *testing.T) { + r := &runner{} + r.IDLabels = []string{"id.label=$value"} + + labels := r.buildServiceLabels(map[string]string{"extra": "it's $here"}) + + if labels["id.label"] != "$$value" { + t.Errorf("id.label = %q, want %q", labels["id.label"], "$$value") + } + if labels["extra"] != `it\'\'s $$here` { + t.Errorf("extra = %q, want %q", labels["extra"], `it\'\'s $$here`) + } + }) +} + +func TestResolveServiceEntrypoint(t *testing.T) { + override := true + t.Run("override command clears entrypoint and command", func(t *testing.T) { + entry, cmd := resolveServiceEntrypoint( + &config.MergedDevContainerConfig{ + DevContainerConfigBase: config.DevContainerConfigBase{OverrideCommand: &override}, + }, + &composetypes.ServiceConfig{Entrypoint: []string{"a"}, Command: []string{"b"}}, + &config.ImageDetails{}, + ) + if len(entry) != 0 || len(cmd) != 0 { + t.Errorf("expected empty entrypoint/command, got %v / %v", entry, cmd) + } + }) + + t.Run("falls back to image entrypoint and command", func(t *testing.T) { + entry, cmd := resolveServiceEntrypoint( + &config.MergedDevContainerConfig{}, + &composetypes.ServiceConfig{}, + &config.ImageDetails{Config: config.ImageDetailsConfig{ + Entrypoint: []string{"img-entry"}, + Cmd: []string{"img-cmd"}, + }}, + ) + if len(entry) != 1 || entry[0] != "img-entry" { + t.Errorf("entrypoint = %v, want [img-entry]", entry) + } + if len(cmd) != 1 || cmd[0] != "img-cmd" { + t.Errorf("command = %v, want [img-cmd]", cmd) + } + }) +} + +func TestNamedVolumesFromMounts(t *testing.T) { + t.Run("nil when no volume mounts", func(t *testing.T) { + if got := namedVolumesFromMounts([]*config.Mount{{Type: mountTypeBind}}); got != nil { + t.Errorf("expected nil, got %+v", got) + } + }) + + t.Run("collects volume mounts", func(t *testing.T) { + got := namedVolumesFromMounts([]*config.Mount{ + {Type: mountTypeVolume, Source: "data", External: true}, + {Type: mountTypeBind, Source: "/host"}, + }) + if len(got) != 1 { + t.Fatalf("expected 1 volume, got %d", len(got)) + } + v := got["data"] + if v.Name != "data" || !bool(v.External) { + t.Errorf("volume = %+v, want name=data external=true", v) + } + }) +} diff --git a/pkg/devcontainer/compose_up.go b/pkg/devcontainer/compose_up.go new file mode 100644 index 000000000..d0c406692 --- /dev/null +++ b/pkg/devcontainer/compose_up.go @@ -0,0 +1,238 @@ +package devcontainer + +import ( + "fmt" + "os" + "path/filepath" + "reflect" + "strings" + "time" + + composetypes "github.com/compose-spec/compose-go/v2/types" + "github.com/devsy-org/devsy/pkg/compose" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" + "gopkg.in/yaml.v3" +) + +// composeLabelEscaper escapes characters in label values that would otherwise +// trigger shell/compose variable interpolation. A literal replacer is used +// instead of a regex because the substitution is a fixed per-character mapping: +// - "$" -> "$$" so compose does not expand it as a variable reference +// - "'" -> "\'\'" so single quotes survive shell-quoted entrypoints +var composeLabelEscaper = strings.NewReplacer("$", "$$", "'", `\'\'`) + +func escapeComposeLabelValue(value string) string { + return composeLabelEscaper.Replace(value) +} + +func (r *runner) extendedDockerComposeUp(params *composeUpParams) (string, error) { + dockerComposeUpProject := r.generateDockerComposeUpProject(params) + dockerComposeData, err := yaml.Marshal(dockerComposeUpProject) + if err != nil { + return "", err + } + + dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) + err = os.MkdirAll(dockerComposeFolder, 0o750) + if err != nil { + return "", err + } + + dockerComposePath := filepath.Join( + dockerComposeFolder, + fmt.Sprintf("%s-%d.yml", FeaturesStartOverrideFilePrefix, time.Now().Second()), + ) + + log.Debugf( + "Creating docker-compose up %s with content:\n %s", + dockerComposePath, + string(dockerComposeData), + ) + + err = os.WriteFile(dockerComposePath, dockerComposeData, 0o600) + if err != nil { + return "", err + } + return dockerComposePath, nil +} + +func (r *runner) generateDockerComposeUpProject(params *composeUpParams) *composetypes.Project { + mergedConfig := params.mergedConfig + composeService := params.composeService + + userEntrypoint, userCommand := resolveServiceEntrypoint( + mergedConfig, + composeService, + params.imageDetails, + ) + + overrideService := &composetypes.ServiceConfig{ + Name: composeService.Name, + Entrypoint: buildOverrideEntrypoint(mergedConfig, userEntrypoint), + Environment: mappingFromMap(mergedConfig.ContainerEnv), + Init: mergedConfig.Init, + CapAdd: mergedConfig.CapAdd, + SecurityOpt: mergedConfig.SecurityOpt, + Labels: r.buildServiceLabels(params.additionalLabels), + } + + if params.originalImageName != params.overrideImageName { + overrideService.Image = params.overrideImageName + } + + if !reflect.DeepEqual(userCommand, composeService.Command) { + overrideService.Command = userCommand + } + + if mergedConfig.ContainerUser != "" { + overrideService.User = mergedConfig.ContainerUser + } + + if mergedConfig.Privileged != nil { + overrideService.Privileged = *mergedConfig.Privileged + } + + gpuSupportEnabled := r.resolveComposeGPUAvailability(params.composeHelper) + r.configureGPUResources(params.parsedConfig, gpuSupportEnabled, overrideService) + + for _, mount := range mergedConfig.Mounts { + overrideService.Volumes = append( + overrideService.Volumes, + mountToServiceVolumeConfig(mount), + ) + } + + project := &composetypes.Project{ + Services: map[string]composetypes.ServiceConfig{ + overrideService.Name: *overrideService, + }, + Volumes: namedVolumesFromMounts(mergedConfig.Mounts), + } + + return project +} + +// resolveServiceEntrypoint determines the effective entrypoint and command for +// the overridden service. When OverrideCommand is set both are cleared so the +// devcontainer entrypoint takes over; otherwise the service values fall back to +// the image's own entrypoint and command. +func resolveServiceEntrypoint( + mergedConfig *config.MergedDevContainerConfig, + composeService *composetypes.ServiceConfig, + imageDetails *config.ImageDetails, +) (entrypoint, command []string) { + entrypoint = composeService.Entrypoint + command = composeService.Command + + if mergedConfig.OverrideCommand != nil && *mergedConfig.OverrideCommand { + return []string{}, []string{} + } + + if len(entrypoint) == 0 { + entrypoint = imageDetails.Config.Entrypoint + } + if len(command) == 0 { + command = imageDetails.Config.Cmd + } + return entrypoint, command +} + +// buildOverrideEntrypoint wraps the user entrypoint in the devcontainer startup +// shim that runs the configured entrypoint scripts before exec'ing the user +// command. +func buildOverrideEntrypoint( + mergedConfig *config.MergedDevContainerConfig, + userEntrypoint []string, +) composetypes.ShellCommand { + entrypoint := composetypes.ShellCommand{ + "/bin/sh", + "-c", + `echo Container started +trap "exit 0" 15 +` + strings.Join(mergedConfig.Entrypoints, "\n") + ` +exec "$$@" +` + DefaultEntrypoint, + "-", + } + return append(entrypoint, userEntrypoint...) +} + +// buildServiceLabels assembles the labels for the overridden service. ID labels +// (or the default ID label) identify the workspace, and any additional labels +// are merged in. All values are escaped to prevent compose variable expansion. +func (r *runner) buildServiceLabels(additionalLabels map[string]string) composetypes.Labels { + labels := composetypes.Labels{} + if len(r.IDLabels) > 0 { + for _, l := range r.IDLabels { + k, v, _ := strings.Cut(l, "=") + labels[k] = escapeComposeLabelValue(v) + } + } else { + labels[config.DockerIDLabel] = r.ID + } + for k, v := range additionalLabels { + labels.Add(k, escapeComposeLabelValue(v)) + } + return labels +} + +// namedVolumesFromMounts collects the named volumes referenced by the merged +// mounts so they can be declared at the project level. Returns nil when no +// volume-type mounts are present. +func namedVolumesFromMounts(mounts []*config.Mount) map[string]composetypes.VolumeConfig { + var volumes map[string]composetypes.VolumeConfig + for _, m := range mounts { + if m.Type != composetypes.VolumeTypeVolume { + continue + } + if volumes == nil { + volumes = map[string]composetypes.VolumeConfig{} + } + volumes[m.Source] = composetypes.VolumeConfig{ + Name: m.Source, + External: composetypes.External(m.External), + } + } + return volumes +} + +func (r *runner) resolveComposeGPUAvailability(composeHelper *compose.ComposeHelper) bool { + switch r.WorkspaceConfig.CLIOptions.GPUAvailability { + case stringTrue: + return true + case stringFalse: + return false + default: + available, _ := composeHelper.Docker.GPUSupportEnabled() + return available + } +} + +func (r *runner) configureGPUResources( + parsedConfig *config.SubstitutedConfig, + gpuSupportEnabled bool, + overrideService *composetypes.ServiceConfig, +) { + if parsedConfig.Config.HostRequirements != nil { + enableGPU, warnIfMissing := parsedConfig.Config.HostRequirements.ShouldEnableGPU( + gpuSupportEnabled, + ) + if enableGPU { + overrideService.Deploy = &composetypes.DeployConfig{ + Resources: composetypes.Resources{ + Reservations: &composetypes.Resource{ + Devices: []composetypes.DeviceRequest{ + { + Capabilities: []string{"gpu"}, + }, + }, + }, + }, + } + } + if warnIfMissing { + log.Warn("GPU required but not available on host") + } + } +} diff --git a/pkg/devcontainer/run.go b/pkg/devcontainer/run.go index 1ace5901e..e1f0369dd 100644 --- a/pkg/devcontainer/run.go +++ b/pkg/devcontainer/run.go @@ -28,14 +28,7 @@ type Runner interface { Find(ctx context.Context) (*config.ContainerDetails, error) - Command( - ctx context.Context, - user string, - command string, - stdin io.Reader, - stdout io.Writer, - stderr io.Writer, - ) error + Command(ctx context.Context, params CommandParams) error Stop(ctx context.Context) error @@ -48,6 +41,16 @@ type DeleteOptions struct { RemoveVolumes bool } +// CommandParams groups the inputs for running a command inside the dev +// container. ctx is passed separately to keep it out of the struct. +type CommandParams struct { + User string + Command string + Stdin io.Reader + Stdout io.Writer + Stderr io.Writer +} + func NewRunner( agentPath, agentDownloadURL string, workspaceConfig *provider2.AgentWorkspaceInfo, @@ -91,6 +94,16 @@ type UpOptions struct { RegistryCache string } +// runContainerParams groups the inputs shared by the runSingleContainer, +// runDockerCompose, and runDefaultContainer dispatch methods. ctx is passed +// separately to keep it out of the struct. +type runContainerParams struct { + parsedConfig *config.SubstitutedConfig + substitutionContext *config.SubstitutionContext + options UpOptions + timeout time.Duration +} + func (r *runner) Up( ctx context.Context, options UpOptions, @@ -121,31 +134,55 @@ func (r *runner) Up( log.Info("Skipping initializeCommand on platform") } + runParams := &runContainerParams{ + parsedConfig: substitutedConfig, + substitutionContext: substitutionContext, + options: options, + timeout: timeout, + } + switch { case isDockerFileConfig(substitutedConfig.Config), substitutedConfig.Config.Image != "", substitutedConfig.Config.ContainerID != "": - return r.runSingleContainer( - ctx, - substitutedConfig, - substitutionContext, - options, - timeout, - ) + return r.runSingleContainer(ctx, runParams) case isDockerComposeConfig(substitutedConfig.Config): - return r.runDockerCompose(ctx, substitutedConfig, substitutionContext, options, timeout) + return r.runDockerCompose(ctx, runParams) default: - return r.runDefaultContainer(ctx, options, substitutedConfig, substitutionContext, timeout) + return r.runDefaultContainer(ctx, runParams) + } +} + +func (r *runner) Command(ctx context.Context, params CommandParams) error { + return r.Driver.CommandDevContainer(ctx, &driver.CommandParams{ + WorkspaceID: r.ID, + User: params.User, + Command: params.Command, + Stdin: params.Stdin, + Stdout: params.Stdout, + Stderr: params.Stderr, + }) +} + +func (r *runner) Find(ctx context.Context) (*config.ContainerDetails, error) { + containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) + if err != nil { + return nil, fmt.Errorf("find dev container: %w", err) } + + return containerDetails, nil +} + +func (r *runner) Logs(ctx context.Context, writer io.Writer) error { + return r.Driver.GetDevContainerLogs(ctx, r.ID, writer, writer) } func (r *runner) runDefaultContainer( ctx context.Context, - options UpOptions, - substitutedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - timeout time.Duration, + params *runContainerParams, ) (*config.Result, error) { + options := params.options + substitutedConfig := params.parsedConfig if options.FallbackImage != "" { log.Warn( "dev container config is missing one of \"image\", \"dockerFile\" or \"dockerComposeFile\" properties, " + @@ -178,38 +215,7 @@ func (r *runner) runDefaultContainer( substitutedConfig.Config.ImageContainer = language.MapConfig[lang].ImageContainer } - return r.runSingleContainer(ctx, substitutedConfig, substitutionContext, options, timeout) -} - -func (r *runner) Command( - ctx context.Context, - user string, - command string, - stdin io.Reader, - stdout io.Writer, - stderr io.Writer, -) error { - return r.Driver.CommandDevContainer(ctx, &driver.CommandParams{ - WorkspaceID: r.ID, - User: user, - Command: command, - Stdin: stdin, - Stdout: stdout, - Stderr: stderr, - }) -} - -func (r *runner) Find(ctx context.Context) (*config.ContainerDetails, error) { - containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) - if err != nil { - return nil, fmt.Errorf("find dev container: %w", err) - } - - return containerDetails, nil -} - -func (r *runner) Logs(ctx context.Context, writer io.Writer) error { - return r.Driver.GetDevContainerLogs(ctx, r.ID, writer, writer) + return r.runSingleContainer(ctx, params) } func isDockerFileConfig(config *config.DevContainerConfig) bool { diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 34230f4e5..28ae12e1c 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -6,7 +6,6 @@ import ( "fmt" "maps" "strings" - "time" "github.com/devsy-org/devsy/pkg/agent/delivery" "github.com/devsy-org/devsy/pkg/command" @@ -52,55 +51,32 @@ type resolveParams struct { func (r *runner) runSingleContainer( ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - options UpOptions, - timeout time.Duration, + runParams *runContainerParams, ) (*config.Result, error) { + parsedConfig := runParams.parsedConfig + substitutionContext := runParams.substitutionContext + options := runParams.options + timeout := runParams.timeout + log.Debugf("starting devcontainer for workspace %s", r.ID) substitutionContext.Userns = options.Userns substitutionContext.UidMap = options.UidMap substitutionContext.GidMap = options.GidMap - // Check if Docker exists before trying to find containers - var containerDetails *config.ContainerDetails - var err error - dockerCmd := "docker" - if r.WorkspaceConfig.Agent.Docker.Path != "" { - dockerCmd = r.WorkspaceConfig.Agent.Docker.Path - } - if command.Exists(dockerCmd) { - containerDetails, err = r.Driver.FindDevContainer(ctx, r.ID) - if err != nil { - return nil, fmt.Errorf("find dev container: %w", err) - } + containerDetails, err := r.findExistingDevContainer(ctx) + if err != nil { + return nil, err } - // Resolve container: ensure we have a running container with merged config. - var resolved *resolvedContainer - params := &resolveParams{ parsedConfig: parsedConfig, substitutionContext: substitutionContext, options: options, } - if options.Recreate && parsedConfig.Config.ContainerID != "" { - return nil, fmt.Errorf("cannot recreate container not created by Devsy") - } else if !options.Recreate && containerDetails != nil { - if actual := workspaceMountDestination(containerDetails); actual != "" && - actual != substitutionContext.ContainerWorkspaceFolder { - log.Infof( - "container workspace mount is %s, updating from computed %s", - actual, substitutionContext.ContainerWorkspaceFolder, - ) - substitutionContext.ContainerWorkspaceFolder = actual - } - resolved, err = r.resolveExistingContainer(ctx, containerDetails, params) - } else { - resolved, err = r.resolveNewContainer(ctx, params) - } + // Resolve container: ensure we have a running container with merged config. + resolved, err := r.resolveContainer(ctx, params, containerDetails) if err != nil { return nil, err } @@ -115,6 +91,57 @@ func (r *runner) runSingleContainer( }) } +// resolveContainer ensures a running container with merged config, either by +// reusing the existing container or creating a new one. Recreating a container +// not created by Devsy (i.e. one with an explicit ContainerID) is rejected. +func (r *runner) resolveContainer( + ctx context.Context, + params *resolveParams, + containerDetails *config.ContainerDetails, +) (*resolvedContainer, error) { + options := params.options + + if options.Recreate && params.parsedConfig.Config.ContainerID != "" { + return nil, fmt.Errorf("cannot recreate container not created by Devsy") + } + + if options.Recreate || containerDetails == nil { + return r.resolveNewContainer(ctx, params) + } + + substitutionContext := params.substitutionContext + if actual := workspaceMountDestination(containerDetails); actual != "" && + actual != substitutionContext.ContainerWorkspaceFolder { + log.Infof( + "container workspace mount is %s, updating from computed %s", + actual, substitutionContext.ContainerWorkspaceFolder, + ) + substitutionContext.ContainerWorkspaceFolder = actual + } + return r.resolveExistingContainer(ctx, containerDetails, params) +} + +// findExistingDevContainer looks up the dev container, first checking that the +// configured docker command exists. Returns nil details (without error) when +// docker is unavailable. +func (r *runner) findExistingDevContainer( + ctx context.Context, +) (*config.ContainerDetails, error) { + dockerCmd := "docker" + if r.WorkspaceConfig.Agent.Docker.Path != "" { + dockerCmd = r.WorkspaceConfig.Agent.Docker.Path + } + if !command.Exists(dockerCmd) { + return nil, nil + } + + containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) + if err != nil { + return nil, fmt.Errorf("find dev container: %w", err) + } + return containerDetails, nil +} + // resolveExistingContainer handles the case where a container already exists. // It starts the container if stopped, merges configuration from container // metadata, and optionally reprovisions. Returns fresh container details. @@ -155,7 +182,7 @@ func (r *runner) ensureRunning( ctx context.Context, containerDetails *config.ContainerDetails, ) (*config.ContainerDetails, error) { - if strings.ToLower(containerDetails.State.Status) == "running" { + if strings.ToLower(containerDetails.State.Status) == containerStatusRunning { return containerDetails, nil } @@ -231,18 +258,49 @@ func (r *runner) resolveNewContainer( ctx context.Context, p *resolveParams, ) (*resolvedContainer, error) { - hostWarnings, hostErr := config.ValidateHostRequirements( - p.parsedConfig.Config.HostRequirements, - config.SystemHostInfo{}, - p.substitutionContext.LocalWorkspaceFolder, - ) - if hostErr != nil { - if !p.options.SkipHostRequirements { - return nil, hostErr + hostWarnings, err := r.newContainerHostWarnings(p) + if err != nil { + return nil, err + } + + buildInfo, mergedConfig, err := r.buildNewContainerConfig(ctx, p) + if err != nil { + return nil, err + } + + r.injectDaemonEntrypoint(p, mergedConfig) + + runOptions, err := r.buildRunOptionsForDelivery(mergedConfig, p.substitutionContext, buildInfo) + if err == nil { + if preStartErr := r.deliverPreStart(ctx, runOptions); preStartErr != nil { + log.Debugf("pre-start delivery skipped or failed, will use post-start: %v", preStartErr) } - hostWarnings = append(hostWarnings, hostErr.Error()) } + err = r.runContainer(ctx, p, mergedConfig, buildInfo) + if err != nil { + return nil, fmt.Errorf("runner run container: %w", err) + } + + containerDetails, err := r.findRunningContainerOrFail(ctx, "creation") + if err != nil { + return nil, err + } + + return &resolvedContainer{ + details: containerDetails, + mergedConfig: mergedConfig, + hostWarnings: hostWarnings, + }, nil +} + +// buildNewContainerConfig builds the image (deleting the existing container +// first when recreating) and produces the merged devcontainer config from the +// build's image metadata. +func (r *runner) buildNewContainerConfig( + ctx context.Context, + p *resolveParams, +) (*config.BuildInfo, *config.MergedDevContainerConfig, error) { buildInfo, err := r.build(ctx, p.parsedConfig, p.substitutionContext, provider2.BuildOptions{ CLIOptions: provider2.CLIOptions{ PrebuildRepositories: p.options.PrebuildRepositories, @@ -255,12 +313,12 @@ func (r *runner) resolveNewContainer( ExportCache: false, }) if err != nil { - return nil, fmt.Errorf("build image: %w", err) + return nil, nil, fmt.Errorf("build image: %w", err) } if p.options.Recreate { if err := r.deleteForRecreate(ctx); err != nil { - return nil, err + return nil, nil, err } } @@ -269,39 +327,35 @@ func (r *runner) resolveNewContainer( buildInfo.ImageMetadata.Config, ) if err != nil { - return nil, fmt.Errorf("merge config: %w", err) + return nil, nil, fmt.Errorf("merge config: %w", err) } if err := config.MergeExtraRemoteEnv( - mergedConfig, p.options.ExtraDevContainerPath, + mergedConfig, + p.options.ExtraDevContainerPath, ); err != nil { - return nil, err + return nil, nil, err } - r.injectDaemonEntrypoint(p, mergedConfig) + return buildInfo, mergedConfig, nil +} - runOptions, err := r.buildRunOptionsForDelivery(mergedConfig, p.substitutionContext, buildInfo) - if err == nil { - if preStartErr := r.deliverPreStart(ctx, runOptions); preStartErr != nil { - log.Debugf("pre-start delivery skipped or failed, will use post-start: %v", preStartErr) +// newContainerHostWarnings validates host requirements for a new container, +// returning warnings. Unmet requirements error unless SkipHostRequirements is +// set, in which case the error is downgraded to a warning. +func (r *runner) newContainerHostWarnings(p *resolveParams) ([]string, error) { + hostWarnings, hostErr := config.ValidateHostRequirements( + p.parsedConfig.Config.HostRequirements, + config.SystemHostInfo{}, + p.substitutionContext.LocalWorkspaceFolder, + ) + if hostErr != nil { + if !p.options.SkipHostRequirements { + return nil, hostErr } + hostWarnings = append(hostWarnings, hostErr.Error()) } - - err = r.runContainer(ctx, p.parsedConfig, p.substitutionContext, mergedConfig, buildInfo) - if err != nil { - return nil, fmt.Errorf("runner run container: %w", err) - } - - containerDetails, err := r.findRunningContainerOrFail(ctx, "creation") - if err != nil { - return nil, err - } - - return &resolvedContainer{ - details: containerDetails, - mergedConfig: mergedConfig, - hostWarnings: hostWarnings, - }, nil + return hostWarnings, nil } // deleteForRecreate removes the existing container before recreating it. @@ -416,8 +470,7 @@ func (r *runner) deliverPreStart(ctx context.Context, runOptions *driver.RunOpti func (r *runner) runContainer( ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, + p *resolveParams, mergedConfig *config.MergedDevContainerConfig, buildInfo *config.BuildInfo, ) error { @@ -426,13 +479,13 @@ func (r *runner) runContainer( // build run options for dockerless mode var runOptions *driver.RunOptions if buildInfo.Dockerless != nil { - runOptions, err = r.getDockerlessRunOptions(mergedConfig, substitutionContext, buildInfo) + runOptions, err = r.getDockerlessRunOptions(mergedConfig, p.substitutionContext, buildInfo) if err != nil { return fmt.Errorf("build dockerless run options: %w", err) } } else { // build run options - runOptions, err = r.getRunOptions(mergedConfig, substitutionContext, buildInfo) + runOptions, err = r.getRunOptions(mergedConfig, p.substitutionContext, buildInfo) if err != nil { return fmt.Errorf("build run options: %w", err) } @@ -446,7 +499,7 @@ func (r *runner) runContainer( return dockerDriver.RunDockerDevContainer(ctx, &driver.RunDockerDevContainerParams{ WorkspaceID: r.ID, Options: runOptions, - ParsedConfig: parsedConfig.Config, + ParsedConfig: p.parsedConfig.Config, IDE: r.WorkspaceConfig.Workspace.IDE.Name, IDEOptions: r.WorkspaceConfig.Workspace.IDE.Options, LocalWorkspaceFolder: r.LocalWorkspaceFolder, @@ -458,25 +511,32 @@ func (r *runner) runContainer( return r.Driver.RunDevContainer(ctx, r.ID, runOptions) } -func (r *runner) getDockerlessRunOptions( - mergedConfig *config.MergedDevContainerConfig, - substitutionContext *config.SubstitutionContext, - buildInfo *config.BuildInfo, -) (*driver.RunOptions, error) { - // parse workspace mount — nil when suppressed via workspaceMount: "" - var workspaceMountPtr *config.Mount - if substitutionContext.WorkspaceMount != "" { - parsed := config.ParseMount(substitutionContext.WorkspaceMount) - workspaceMountPtr = &parsed +// parseWorkspaceMount parses the substituted workspace mount, returning nil when +// it has been suppressed via an empty workspaceMount. +func parseWorkspaceMount(substitutionContext *config.SubstitutionContext) *config.Mount { + if substitutionContext.WorkspaceMount == "" { + return nil } + parsed := config.ParseMount(substitutionContext.WorkspaceMount) + return &parsed +} - // add metadata as label here - marshalled, err := metadata.MarshalImageMetadata(buildInfo.ImageMetadata.Raw) - if err != nil { - return nil, fmt.Errorf("marshal config: %w", err) +// workspaceUID returns the workspace UID, or an empty string when unavailable. +func (r *runner) workspaceUID() string { + if r.WorkspaceConfig != nil && r.WorkspaceConfig.Workspace != nil { + return r.WorkspaceConfig.Workspace.UID } + return "" +} + +// dockerlessEnv builds the environment for a dockerless build container, +// combining the kaniko/dockerless settings with the merged container env. +func (r *runner) dockerlessEnv( + mergedConfig *config.MergedDevContainerConfig, + buildInfo *config.BuildInfo, +) (map[string]string, error) { env := map[string]string{ - "DOCKERLESS": "true", + "DOCKERLESS": stringTrue, "DOCKERLESS_CONTEXT": buildInfo.Dockerless.Context, "DOCKERLESS_DOCKERFILE": buildInfo.Dockerless.Dockerfile, "GODEBUG": "http2client=0", // https://github.com/GoogleContainerTools/kaniko/issues/875 @@ -490,9 +550,28 @@ func (r *runner) getDockerlessRunOptions( if err != nil { return nil, fmt.Errorf("marshal build args: %w", err) } - env["DOCKERLESS_BUILD_ARGS"] = string(out) } + return env, nil +} + +func (r *runner) getDockerlessRunOptions( + mergedConfig *config.MergedDevContainerConfig, + substitutionContext *config.SubstitutionContext, + buildInfo *config.BuildInfo, +) (*driver.RunOptions, error) { + workspaceMountPtr := parseWorkspaceMount(substitutionContext) + + // add metadata as label here + marshalled, err := metadata.MarshalImageMetadata(buildInfo.ImageMetadata.Raw) + if err != nil { + return nil, fmt.Errorf("marshal config: %w", err) + } + + env, err := r.dockerlessEnv(mergedConfig, buildInfo) + if err != nil { + return nil, err + } image := dockerlessImage if r.WorkspaceConfig != nil && r.WorkspaceConfig.Agent.Dockerless.Image != "" { @@ -507,16 +586,11 @@ func (r *runner) getDockerlessRunOptions( Target: "/workspaces/.dockerless", }) - uid := "" - if r.WorkspaceConfig != nil && r.WorkspaceConfig.Workspace != nil { - uid = r.WorkspaceConfig.Workspace.UID - } - // build run options return &driver.RunOptions{ - UID: uid, + UID: r.workspaceUID(), Image: image, - User: "root", + User: containerRootUser, Entrypoint: "/.dockerless/dockerless", Cmd: []string{ "start", @@ -548,12 +622,7 @@ func (r *runner) getRunOptions( substitutionContext *config.SubstitutionContext, buildInfo *config.BuildInfo, ) (*driver.RunOptions, error) { - // parse workspace mount — nil when suppressed via workspaceMount: "" - var workspaceMountPtr *config.Mount - if substitutionContext.WorkspaceMount != "" { - parsed := config.ParseMount(substitutionContext.WorkspaceMount) - workspaceMountPtr = &parsed - } + workspaceMountPtr := parseWorkspaceMount(substitutionContext) // add metadata as label here marshalled, err := metadata.MarshalImageMetadata(buildInfo.ImageMetadata.Raw) @@ -580,11 +649,6 @@ func (r *runner) getRunOptions( user = mergedConfig.ContainerUser } - uid := "" - if r.WorkspaceConfig != nil && r.WorkspaceConfig.Workspace != nil { - uid = r.WorkspaceConfig.Workspace.UID - } - // Resolve any ${containerEnv:VAR} references in containerEnv values using // the image's inspected environment. ${containerWorkspaceFolder} and // ${containerWorkspaceFolderBasename} are already substituted upstream in @@ -602,7 +666,7 @@ func (r *runner) getRunOptions( mergedConfig.ContainerEnv = resolvedContainerEnv return &driver.RunOptions{ - UID: uid, + UID: r.workspaceUID(), Image: buildInfo.ImageName, User: user, Entrypoint: entrypoint, @@ -629,8 +693,8 @@ func (r *runner) addExtraEnvVars(env map[string]string) map[string]string { env = make(map[string]string) } - env[DevsyExtraEnvVar] = "true" - env[RemoteContainersExtraEnvVar] = "true" + env[DevsyExtraEnvVar] = stringTrue + env[RemoteContainersExtraEnvVar] = stringTrue if r.WorkspaceConfig != nil && r.WorkspaceConfig.Workspace != nil && r.WorkspaceConfig.Workspace.ID != "" { env[pkgconfig.EnvWorkspaceID] = r.WorkspaceConfig.Workspace.ID From d681f08c09e36f447f3eb5a7a68c255768e836ba Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 06:07:37 -0500 Subject: [PATCH 02/13] fix(devcontainer): address review findings from PR #546 - Preserve Dockerfile contents when a compose build.target is set, so a build-backed service is not misclassified as image-based (would emit 'FROM AS '). - Fall back to a full start when an existing-project 'compose up' reports success but no dev container is found, instead of finalizing a nil container. - Use os.CreateTemp for compose override files instead of time.Now().Second(), avoiding filename collisions across builds/starts in the same second (shared writeComposeOverrideFile helper; prefix retained so checkForPersistedFile still matches). - Skip anonymous volumes (empty source) when declaring project-level volumes, keeping them service-scoped. - Close the log.Writer in updateContainerUserUID to avoid a writer leak. Skipped: nil-deref guard (GetExtendedBuildInfo never returns (nil,nil) on success), the '\'' label escaping (matches upstream devcontainers-cli), and the docker-CLI-absent guard (intentional). All pre-existing on main. Adds anonymous-volume regression test. 199 tests pass. --- pkg/devcontainer/compose.go | 59 ++++++++++++++++++++++++++----- pkg/devcontainer/compose_build.go | 22 +++++------- pkg/devcontainer/compose_test.go | 13 +++++++ pkg/devcontainer/compose_up.go | 23 ++++-------- 4 files changed, 80 insertions(+), 37 deletions(-) diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 06b8e9c2c..3f46c5d48 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -372,11 +372,13 @@ func (r *runner) updateContainerUserUID( if !ok { return nil } + writer := log.Writer(log.LevelInfo) + defer func() { _ = writer.Close() }() if err := dockerDriver.UpdateContainerUserUID( ctx, r.ID, parsedConfig.Config, - log.Writer(log.LevelInfo), + writer, ); err != nil { log.Errorf("failed to update container user UID/GID: error=%v", err) return err @@ -444,7 +446,24 @@ func (r *runner) tryStartExistingProject( // If project is found, we can call `up` with the project name. // If it fails, fall back to rebuilding. - upArgs := []string{composeProjectNameFlag, project.Name} + details, err := r.composeUpExistingProject(ctx, params, existingProjectFiles) + if err != nil || details == nil { + // Compose failed, or reported success but the dev container is not + // present; fall back to a full start rather than finalizing nil. + return containerDetails, false + } + + return details, true +} + +// composeUpExistingProject runs "compose up" using the persisted project files +// and returns the resulting dev container details. +func (r *runner) composeUpExistingProject( + ctx context.Context, + params *existingProjectParams, + existingProjectFiles []string, +) (*config.ContainerDetails, error) { + upArgs := []string{composeProjectNameFlag, params.project.Name} for _, projectFile := range existingProjectFiles { upArgs = append(upArgs, "-f", projectFile) } @@ -452,23 +471,24 @@ func (r *runner) tryStartExistingProject( upArgs = r.onlyRunServices(upArgs, params.parsedConfig) writer := log.Writer(log.LevelInfo) - if err := composeHelper.Run(ctx, upArgs, nil, writer, writer); err != nil { + defer func() { _ = writer.Close() }() + if err := params.composeHelper.Run(ctx, upArgs, nil, writer, writer); err != nil { log.Errorf("Error starting project: %s", err) - return containerDetails, false + return nil, err } // wait for running and get container details - details, err := composeHelper.FindDevContainer( + details, err := params.composeHelper.FindDevContainer( ctx, - project.Name, + params.project.Name, params.parsedConfig.Config.Service, ) if err != nil { log.Errorf("Error finding dev container: %s", err) - return containerDetails, false + return nil, err } - return details, true + return details, nil } // allProjectFilesExist reports whether every persisted project file is still @@ -915,6 +935,29 @@ func getDockerComposeFolder(workspaceOriginFolder string) string { return filepath.Join(workspaceOriginFolder, ".docker-compose") } +// writeComposeOverrideFile writes a compose override file into the workspace's +// docker-compose folder using a collision-safe unique name that retains the +// given prefix (so checkForPersistedFile can still match it by prefix). +func (r *runner) writeComposeOverrideFile(prefix string, data []byte) (string, error) { + dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) + if err := os.MkdirAll(dockerComposeFolder, 0o750); err != nil { + return "", err + } + + f, err := os.CreateTemp(dockerComposeFolder, prefix+"-*.yml") + if err != nil { + return "", err + } + defer func() { _ = f.Close() }() + + if _, err := f.Write(data); err != nil { + _ = os.Remove(f.Name()) + return "", err + } + + return f.Name(), nil +} + func mappingFromMap(m map[string]string) composetypes.MappingWithEquals { if len(m) == 0 { return nil diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 4c870c7ff..5015f8cb7 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -8,7 +8,6 @@ import ( "path/filepath" "regexp" "strings" - "time" composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" @@ -73,6 +72,9 @@ func (r *runner) prepareComposeDockerfileBuildInfo( var buildTarget string if originalTarget != "" { buildTarget = originalTarget + // Preserve the Dockerfile contents so a build-backed service with an + // explicit target is not later misclassified as image-based. + dockerfileContents = string(originalDockerfile) } else { lastStageName, modifiedDockerfile, stageErr := dockerfile.EnsureFinalStageName( string(originalDockerfile), @@ -577,20 +579,18 @@ func (r *runner) writeComposeFile(service *composetypes.ServiceConfig) (string, }, } - dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) - if err := os.MkdirAll(dockerComposeFolder, 0o750); err != nil { - return "", err - } - dockerComposeData, err := yaml.Marshal(project) if err != nil { return "", err } - dockerComposePath := filepath.Join( - dockerComposeFolder, - fmt.Sprintf("%s-%d.yml", FeaturesBuildOverrideFilePrefix, time.Now().Second()), + dockerComposePath, err := r.writeComposeOverrideFile( + FeaturesBuildOverrideFilePrefix, + dockerComposeData, ) + if err != nil { + return "", err + } log.Debugf( "Creating docker-compose build %s with content:\n %s", @@ -598,10 +598,6 @@ func (r *runner) writeComposeFile(service *composetypes.ServiceConfig) (string, string(dockerComposeData), ) - if err := os.WriteFile(dockerComposePath, dockerComposeData, 0o600); err != nil { - return "", err - } - return dockerComposePath, nil } diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 7ddee0154..98b957564 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -572,4 +572,17 @@ func TestNamedVolumesFromMounts(t *testing.T) { t.Errorf("volume = %+v, want name=data external=true", v) } }) + + t.Run("skips anonymous volumes with empty source", func(t *testing.T) { + got := namedVolumesFromMounts([]*config.Mount{ + {Type: mountTypeVolume, Target: "/anon"}, + {Type: mountTypeVolume, Source: "named"}, + }) + if len(got) != 1 { + t.Fatalf("expected 1 named volume, got %d: %+v", len(got), got) + } + if _, ok := got[""]; ok { + t.Error("anonymous volume with empty source should not be declared") + } + }) } diff --git a/pkg/devcontainer/compose_up.go b/pkg/devcontainer/compose_up.go index d0c406692..7a08ffbb0 100644 --- a/pkg/devcontainer/compose_up.go +++ b/pkg/devcontainer/compose_up.go @@ -1,12 +1,8 @@ package devcontainer import ( - "fmt" - "os" - "path/filepath" "reflect" "strings" - "time" composetypes "github.com/compose-spec/compose-go/v2/types" "github.com/devsy-org/devsy/pkg/compose" @@ -33,27 +29,20 @@ func (r *runner) extendedDockerComposeUp(params *composeUpParams) (string, error return "", err } - dockerComposeFolder := getDockerComposeFolder(r.WorkspaceConfig.Origin) - err = os.MkdirAll(dockerComposeFolder, 0o750) + dockerComposePath, err := r.writeComposeOverrideFile( + FeaturesStartOverrideFilePrefix, + dockerComposeData, + ) if err != nil { return "", err } - dockerComposePath := filepath.Join( - dockerComposeFolder, - fmt.Sprintf("%s-%d.yml", FeaturesStartOverrideFilePrefix, time.Now().Second()), - ) - log.Debugf( "Creating docker-compose up %s with content:\n %s", dockerComposePath, string(dockerComposeData), ) - err = os.WriteFile(dockerComposePath, dockerComposeData, 0o600) - if err != nil { - return "", err - } return dockerComposePath, nil } @@ -183,7 +172,9 @@ func (r *runner) buildServiceLabels(additionalLabels map[string]string) composet func namedVolumesFromMounts(mounts []*config.Mount) map[string]composetypes.VolumeConfig { var volumes map[string]composetypes.VolumeConfig for _, m := range mounts { - if m.Type != composetypes.VolumeTypeVolume { + // Only named volumes are declared at the project level; anonymous + // volumes (empty source) stay service-scoped. + if m.Type != composetypes.VolumeTypeVolume || m.Source == "" { continue } if volumes == nil { From 6ff86c47910edc88b69e311c1df2a541f0873804 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 06:14:31 -0500 Subject: [PATCH 03/13] fix(devcontainer): only escape $ in compose label values The label escaper also rewrote ' -> \'\' to survive shell-quoted entrypoints, but devsy's compose labels never pass through a shell: they are written as YAML and the compose CLI is invoked via argv. Compose interpolation only treats $ as special. Escaping ' therefore corrupted the stored payload (e.g. JSON metadata labels containing apostrophes would fail json.Unmarshal on read-back, which does no unescaping). Keep $ -> $$ (round-trips via Compose) and pass all other characters through verbatim. Expands TestEscapeComposeLabelValue with dollar/quote/ unicode/JSON edge cases and adds a round-trip test. Pre-existing on main; not introduced by the split refactor. --- pkg/devcontainer/compose_test.go | 88 +++++++++++++++++++++++++++++--- pkg/devcontainer/compose_up.go | 12 ++--- 2 files changed, 88 insertions(+), 12 deletions(-) diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 98b957564..84c264775 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -478,10 +478,32 @@ func TestEscapeComposeLabelValue(t *testing.T) { want string }{ {name: "plain value untouched", in: "plain-value", want: "plain-value"}, - {name: "dollar doubled", in: "$HOME", want: "$$HOME"}, - {name: "single quote escaped", in: "it's", want: `it\'\'s`}, - {name: "dollar and quote combined", in: "$a'b", want: `$$a\'\'b`}, {name: "empty value", in: "", want: ""}, + + // "$" is the only character Compose interpolates, so it is doubled. + {name: "single dollar doubled", in: "$HOME", want: "$$HOME"}, + {name: "leading dollar", in: "$", want: "$$"}, + {name: "trailing dollar", in: "a$", want: "a$$"}, + {name: "multiple dollars", in: "$a$b$", want: "$$a$$b$$"}, + {name: "already doubled dollar is doubled again", in: "$$", want: "$$$$"}, + {name: "braced variable reference", in: "${FOO}", want: "$${FOO}"}, + + // Characters that are NOT special to Compose interpolation must pass + // through verbatim. Escaping them would corrupt the stored payload. + {name: "single quote preserved", in: "it's", want: "it's"}, + {name: "double quote preserved", in: `say "hi"`, want: `say "hi"`}, + {name: "backslash preserved", in: `a\b`, want: `a\b`}, + {name: "backtick preserved", in: "a`b", want: "a`b"}, + {name: "newline preserved", in: "line1\nline2", want: "line1\nline2"}, + {name: "unicode preserved", in: "café—naïve 🚀", want: "café—naïve 🚀"}, + + // Realistic metadata: JSON containing an apostrophe must survive so it + // can be json.Unmarshal'd back on read. Only "$" is altered. + { + name: "json metadata with apostrophe and dollar", + in: `[{"id":"it's-a-feature","cmd":"echo $X"}]`, + want: `[{"id":"it's-a-feature","cmd":"echo $$X"}]`, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -492,6 +514,34 @@ func TestEscapeComposeLabelValue(t *testing.T) { } } +// TestEscapeComposeLabelValueRoundTrip documents the contract that motivates the +// escaping: doubling "$" survives Compose's interpolation (which un-doubles +// "$$" back to "$"), and non-"$" characters are returned unchanged so a +// JSON-encoded metadata label can be unmarshalled back without corruption. +func TestEscapeComposeLabelValueRoundTrip(t *testing.T) { + original := `{"name":"it's a test","entrypoint":"run $CMD"}` + + escaped := escapeComposeLabelValue(original) + + // Compose interpolation collapses "$$" -> "$"; emulate that to recover the + // value a consumer would see. + interpolated := strings.ReplaceAll(escaped, "$$", "$") + if interpolated != original { + t.Errorf( + "round-trip mismatch:\n escaped = %q\n interpolated = %q\n original = %q", + escaped, + interpolated, + original, + ) + } + + // The apostrophe (and every non-"$" byte) must be untouched in the escaped + // form so json.Unmarshal succeeds after interpolation. + if strings.Contains(escaped, `\'`) { + t.Errorf("apostrophe was escaped, corrupting the payload: %q", escaped) + } +} + func TestBuildServiceLabels(t *testing.T) { t.Run("uses default ID label when no ID labels", func(t *testing.T) { r := &runner{} @@ -504,7 +554,7 @@ func TestBuildServiceLabels(t *testing.T) { } }) - t.Run("escapes ID and additional label values", func(t *testing.T) { + t.Run("escapes dollars but preserves other characters", func(t *testing.T) { r := &runner{} r.IDLabels = []string{"id.label=$value"} @@ -513,8 +563,34 @@ func TestBuildServiceLabels(t *testing.T) { if labels["id.label"] != "$$value" { t.Errorf("id.label = %q, want %q", labels["id.label"], "$$value") } - if labels["extra"] != `it\'\'s $$here` { - t.Errorf("extra = %q, want %q", labels["extra"], `it\'\'s $$here`) + // The apostrophe must be preserved; only "$" is doubled. + if labels["extra"] != "it's $$here" { + t.Errorf("extra = %q, want %q", labels["extra"], "it's $$here") + } + }) + + t.Run("splits ID label only on first equals sign", func(t *testing.T) { + r := &runner{} + r.IDLabels = []string{"id.label=a=b=c"} + + labels := r.buildServiceLabels(nil) + + if labels["id.label"] != "a=b=c" { + t.Errorf("id.label = %q, want %q", labels["id.label"], "a=b=c") + } + }) + + t.Run("additional labels merge alongside ID labels", func(t *testing.T) { + r := &runner{} + r.IDLabels = []string{"id.label=v"} + + labels := r.buildServiceLabels(map[string]string{"k": "plain"}) + + if labels["id.label"] != "v" { + t.Errorf("id.label = %q, want %q", labels["id.label"], "v") + } + if labels["k"] != "plain" { + t.Errorf("k = %q, want %q", labels["k"], "plain") } }) } diff --git a/pkg/devcontainer/compose_up.go b/pkg/devcontainer/compose_up.go index 7a08ffbb0..63822b02a 100644 --- a/pkg/devcontainer/compose_up.go +++ b/pkg/devcontainer/compose_up.go @@ -11,12 +11,12 @@ import ( "gopkg.in/yaml.v3" ) -// composeLabelEscaper escapes characters in label values that would otherwise -// trigger shell/compose variable interpolation. A literal replacer is used -// instead of a regex because the substitution is a fixed per-character mapping: -// - "$" -> "$$" so compose does not expand it as a variable reference -// - "'" -> "\'\'" so single quotes survive shell-quoted entrypoints -var composeLabelEscaper = strings.NewReplacer("$", "$$", "'", `\'\'`) +// composeLabelEscaper escapes "$" as "$$" so Compose does not treat the value +// as a variable reference; Compose un-doubles it back to a literal "$" during +// interpolation. Only "$" is special here: the override is written as YAML and +// the compose CLI is invoked via an argv (no shell), so no other characters +// (e.g. "'") need escaping — doing so would corrupt the stored label payload. +var composeLabelEscaper = strings.NewReplacer("$", "$$") func escapeComposeLabelValue(value string) string { return composeLabelEscaper.Replace(value) From 3eeb80ebbc18529af13da4696e9a4b8cc274558f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 06:18:56 -0500 Subject: [PATCH 04/13] test(e2e): add compose build.target + features regression test Adds an end-to-end guard for the build.target dockerfileContents fix: a build-backed compose service with an explicit build target AND a feature. The multi-stage Dockerfile overwrites /stage-marker.txt in the 'dev' target stage; the test asserts the marker reads 'dev-stage', proving the real Dockerfile and its target were honored rather than a synthesized 'FROM AS '. It also asserts the feature (vcluster) is installed on top of the targeted stage. No existing e2e combined build.target with features. --- .../.devcontainer.json | 11 ++++++ .../Dockerfile | 9 +++++ .../docker-compose.yaml | 11 ++++++ .../up-docker-compose/up_docker_compose.go | 37 +++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/.devcontainer.json create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/Dockerfile create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/docker-compose.yaml diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/.devcontainer.json b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/.devcontainer.json new file mode 100644 index 000000000..13a709164 --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/.devcontainer.json @@ -0,0 +1,11 @@ +{ + "name": "Go", + "dockerComposeFile": "./docker-compose.yaml", + "service": "app", + "workspaceFolder": "/workspaces", + "features": { + "ghcr.io/devsy-org/devcontainer-features/vcluster:1.0.1": { + "version": "v0.24.1" + } + } +} diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/Dockerfile b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/Dockerfile new file mode 100644 index 000000000..dcab83b93 --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/Dockerfile @@ -0,0 +1,9 @@ +FROM ghcr.io/devsy-org/test-images/go:1 AS base + +RUN echo "base-stage" > /stage-marker.txt + +FROM base AS dev + +RUN echo "dev-stage" > /stage-marker.txt +RUN mkdir -p /app +WORKDIR /app diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/docker-compose.yaml b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/docker-compose.yaml new file mode 100644 index 000000000..2564d2ae1 --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-build-target-features/docker-compose.yaml @@ -0,0 +1,11 @@ +version: '3' + +services: + app: + build: + context: . + dockerfile: Dockerfile + target: dev + command: sleep infinity + volumes: + - .:/workspaces:cached diff --git a/e2e/tests/up-docker-compose/up_docker_compose.go b/e2e/tests/up-docker-compose/up_docker_compose.go index 6726b4f96..03136c68c 100644 --- a/e2e/tests/up-docker-compose/up_docker_compose.go +++ b/e2e/tests/up-docker-compose/up_docker_compose.go @@ -334,6 +334,43 @@ var _ = ginkgo.Describe( To(gomega.ContainSubstring("vcluster version 0.24.1")) }, ginkgo.SpecTimeout(framework.TimeoutLong())) + // Regression guard: a build-backed service with an explicit build.target + // and features must honor the real Dockerfile. Previously the Dockerfile + // contents were dropped when a target was set, producing a synthesized + // "FROM AS " that ignored the requested stage (and broke + // build-only services that have no top-level image). + ginkgo.It("features with build target", func(ctx context.Context) { + tempDir, workspace, err := tc.setupAndStartWorkspace( + ctx, + "tests/up-docker-compose/testdata/docker-compose-build-target-features", + "--debug", + ) + framework.ExpectNoError(err) + + ids, err := findComposeContainer( + ctx, + tc.dockerHelper, + tc.composeHelper, + workspace.UID, + "app", + ) + framework.ExpectNoError(err) + gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created") + + // The "dev" stage overwrites the marker, so seeing "dev-stage" + // proves the real multi-stage Dockerfile (and its target) was built. + stageMarker, err := tc.execSSH(ctx, tempDir, "cat /stage-marker.txt") + framework.ExpectNoError(err) + gomega.Expect(strings.TrimSpace(stageMarker)). + To(gomega.Equal("dev-stage"), "the requested build target stage should be used") + + // The feature must still be installed on top of the targeted stage. + vclusterVersionOutput, err := tc.execSSH(ctx, tempDir, "vcluster --version") + framework.ExpectNoError(err) + gomega.Expect(vclusterVersionOutput). + To(gomega.ContainSubstring("vcluster version 0.24.1")) + }, ginkgo.SpecTimeout(framework.TimeoutLong())) + ginkgo.It( "does not retag shared image when applying features to image backed services", func(ctx context.Context) { From b190ca91561b53f41cdf271bc371d730fd0278fc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 06:34:25 -0500 Subject: [PATCH 05/13] docs(devcontainer): tighten verbose comments added in refactor Remove the repeated 'ctx is passed separately' clause (an obvious Go idiom) from three param-struct docs, and compress two step-by-step body narrations and the label-escaper rationale to their essential intent. No behavior change. --- pkg/devcontainer/compose.go | 10 ++++------ pkg/devcontainer/compose_up.go | 9 ++++----- pkg/devcontainer/run.go | 5 ++--- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 3f46c5d48..7dcc7a348 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -52,7 +52,7 @@ type persistedFileResult struct { } // startContainerParams groups the inputs for starting (or recreating) the -// compose dev container. ctx is passed separately to keep it out of the struct. +// compose dev container. type startContainerParams struct { parsedConfig *config.SubstitutedConfig substitutionContext *config.SubstitutionContext @@ -304,9 +304,8 @@ func (r *runner) ensureComposeContainer( return containerDetails, nil } -// finalizeComposeContainer derives the merged config from container metadata, -// updates the container user, exposes the compose project name, validates host -// requirements, and sets up the container. +// finalizeComposeContainer merges the container's metadata config and sets up +// the running container. func (r *runner) finalizeComposeContainer( ctx context.Context, runParams *runContainerParams, @@ -362,8 +361,7 @@ func (r *runner) finalizeComposeContainer( }) } -// updateContainerUserUID updates the container user's UID/GID to match the local -// user when running on a Docker driver. +// updateContainerUserUID updates the container user's UID/GID on Docker drivers. func (r *runner) updateContainerUserUID( ctx context.Context, parsedConfig *config.SubstitutedConfig, diff --git a/pkg/devcontainer/compose_up.go b/pkg/devcontainer/compose_up.go index 63822b02a..885607aef 100644 --- a/pkg/devcontainer/compose_up.go +++ b/pkg/devcontainer/compose_up.go @@ -11,11 +11,10 @@ import ( "gopkg.in/yaml.v3" ) -// composeLabelEscaper escapes "$" as "$$" so Compose does not treat the value -// as a variable reference; Compose un-doubles it back to a literal "$" during -// interpolation. Only "$" is special here: the override is written as YAML and -// the compose CLI is invoked via an argv (no shell), so no other characters -// (e.g. "'") need escaping — doing so would corrupt the stored label payload. +// composeLabelEscaper doubles "$" so Compose does not interpolate it as a +// variable reference. "$" is the only special character: labels are written as +// YAML and the compose CLI runs via argv (no shell), so escaping anything else +// (e.g. "'") would corrupt the stored value. var composeLabelEscaper = strings.NewReplacer("$", "$$") func escapeComposeLabelValue(value string) string { diff --git a/pkg/devcontainer/run.go b/pkg/devcontainer/run.go index e1f0369dd..2b4af6629 100644 --- a/pkg/devcontainer/run.go +++ b/pkg/devcontainer/run.go @@ -42,7 +42,7 @@ type DeleteOptions struct { } // CommandParams groups the inputs for running a command inside the dev -// container. ctx is passed separately to keep it out of the struct. +// container. type CommandParams struct { User string Command string @@ -95,8 +95,7 @@ type UpOptions struct { } // runContainerParams groups the inputs shared by the runSingleContainer, -// runDockerCompose, and runDefaultContainer dispatch methods. ctx is passed -// separately to keep it out of the struct. +// runDockerCompose, and runDefaultContainer dispatch methods. type runContainerParams struct { parsedConfig *config.SubstitutedConfig substitutionContext *config.SubstitutionContext From 213fca1a9d5018117d8ee5545ae6c9482db1910b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 06:52:08 -0500 Subject: [PATCH 06/13] chore(deps): regenerate THIRD_PARTY_LICENSES.md The continuity (v0.5.0) and fsutil dependency bumps that landed on main updated go.mod but not the attribution file, so the licenses CI check failed on the merge. Regenerated via 'go run ./hack/licenses'. --- THIRD_PARTY_LICENSES.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/THIRD_PARTY_LICENSES.md b/THIRD_PARTY_LICENSES.md index 821841d04..2e80c698f 100644 --- a/THIRD_PARTY_LICENSES.md +++ b/THIRD_PARTY_LICENSES.md @@ -75,7 +75,7 @@ To regenerate this file after changing dependencies, run `task cli:licenses`. | [github.com/containerd/console](https://github.com/containerd/console) | `v1.0.5` | Apache-2.0 | | [github.com/containerd/containerd/api](https://github.com/containerd/containerd) | `v1.10.0` | Apache-2.0 | | [github.com/containerd/containerd/v2](https://github.com/containerd/containerd) | `v2.2.2` | Apache-2.0 | -| [github.com/containerd/continuity](https://github.com/containerd/continuity) | `v0.4.5` | Apache-2.0 | +| [github.com/containerd/continuity](https://github.com/containerd/continuity) | `v0.5.0` | Apache-2.0 | | [github.com/containerd/errdefs](https://github.com/containerd/errdefs) | `v1.0.0` | Apache-2.0 | | [github.com/containerd/errdefs/pkg](https://github.com/containerd/errdefs) | `v0.3.0` | Apache-2.0 | | [github.com/containerd/log](https://github.com/containerd/log) | `v0.1.0` | Apache-2.0 | @@ -243,7 +243,7 @@ To regenerate this file after changing dependencies, run `task cli:licenses`. | [github.com/tidwall/pretty](https://github.com/tidwall/pretty) | `v1.2.1` | MIT | | [github.com/tklauser/go-sysconf](https://github.com/tklauser/go-sysconf) | `v0.3.16` | BSD-3-Clause | | [github.com/tklauser/numcpus](https://github.com/tklauser/numcpus) | `v0.11.0` | Apache-2.0 | -| [github.com/tonistiigi/fsutil](https://github.com/tonistiigi/fsutil) | `v0.0.0-20251211185533-a2aa163d723f` | MIT | +| [github.com/tonistiigi/fsutil](https://github.com/tonistiigi/fsutil) | `v0.0.0-20260609174605-b61e79c0c046` | MIT | | [github.com/tonistiigi/go-csvvalue](https://github.com/tonistiigi/go-csvvalue) | `v0.0.0-20240814133006-030d3b2625d0` | MIT | | [github.com/tonistiigi/units](https://github.com/tonistiigi/units) | `v0.0.0-20180711220420-6950e57a87ea` | MIT | | [github.com/tonistiigi/vt100](https://github.com/tonistiigi/vt100) | `v0.0.0-20240514184818-90bafcd6abab` | MIT | From ae23e7023c4bded1d7e4cec8bac8a380b7859fe8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 16:33:06 -0500 Subject: [PATCH 07/13] fix(devcontainer): address PR #546 follow-up review Three findings, all independently verified as pre-existing on main: - tryStartExistingProject: short-circuit to a full rebuild when a persisted project file is missing, instead of running 'compose up -f ' which only fails and falls back anyway (Copilot). - parseTmpfsSize: reject negative sizes; ParseInt accepts '-1' and would otherwise forward a bogus value into the override (CodeRabbit). Adds a regression test. - buildAndExtendDockerCompose: GetExtendedBuildInfo never returns nil on success (verified: every nil return carries a non-nil error, handled before this point), so the '--pull' gate on extendImageBuildInfo == nil was dead. Set pull: false to preserve behavior and document why, rather than speculatively enabling --pull on featureless builds (Copilot). Same non-nil guarantee confirms the earlier 'nil-deref' finding remains a false positive. --- pkg/devcontainer/compose.go | 8 +++++--- pkg/devcontainer/compose_build.go | 14 +++++++++----- pkg/devcontainer/compose_mounts.go | 4 ++-- pkg/devcontainer/compose_test.go | 5 +++++ 4 files changed, 21 insertions(+), 10 deletions(-) diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 7dcc7a348..ce611cd81 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -439,11 +439,13 @@ func (r *runner) tryStartExistingProject( log.Debugf("Found existing project files: %s", existingProjectFiles) if !allProjectFilesExist(existingProjectFiles) { - containerDetails = nil + // A referenced file is gone, so `compose up -f ` would only + // fail; rebuild from scratch instead. + return containerDetails, false } - // If project is found, we can call `up` with the project name. - // If it fails, fall back to rebuilding. + // The project files are present, so `up` can reuse them. If it fails, the + // caller falls back to rebuilding. details, err := r.composeUpExistingProject(ctx, params, existingProjectFiles) if err != nil || details == nil { // Compose failed, or reported success but the dev container is not diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 5015f8cb7..0bf232c8d 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -145,9 +145,13 @@ func (r *runner) buildAndExtendDockerCompose( projectName: params.project.Name, globalArgs: params.globalArgs, overrideComposeFilePath: dockerComposeFilePath, - pull: extendImageBuildInfo == nil, - serviceName: params.composeService.Name, - runServices: params.parsedConfig.Config.RunServices, + // extendImageBuildInfo is non-nil here (a nil result always comes with + // an error, handled above), so --pull is effectively never added. This + // preserves the original behavior; do not switch to hasFeatureBuildInfo + // without intending to start passing --pull on featureless builds. + pull: false, + serviceName: params.composeService.Name, + runServices: params.parsedConfig.Config.RunServices, }) if err := r.runComposeBuild(ctx, params.composeHelper, buildArgs); err != nil { @@ -342,8 +346,8 @@ type composeBuildArgsParams struct { } // composeBuildArgs assembles the "docker compose ... build" argument list, -// adding the override file, --pull when no feature build info is present, and -// any explicitly requested run services. +// adding the override file, --pull when params.pull is set, and any explicitly +// requested run services. func composeBuildArgs(params *composeBuildArgsParams) []string { buildArgs := []string{composeProjectNameFlag, params.projectName} buildArgs = append(buildArgs, params.globalArgs...) diff --git a/pkg/devcontainer/compose_mounts.go b/pkg/devcontainer/compose_mounts.go index dfb6d29bd..679201067 100644 --- a/pkg/devcontainer/compose_mounts.go +++ b/pkg/devcontainer/compose_mounts.go @@ -66,8 +66,8 @@ func parseTmpfsSize(raw, target string) (composetypes.UnitBytes, bool) { return 0, false } parsed, err := strconv.ParseInt(raw, 10, 64) - if err != nil { - log.Warnf("ignoring tmpfs-size %q on mount %s: %s", raw, target, err) + if err != nil || parsed < 0 { + log.Warnf("ignoring invalid tmpfs-size %q on mount %s", raw, target) return 0, false } return composetypes.UnitBytes(parsed), true diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 84c264775..c950bcabc 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -457,6 +457,11 @@ func TestTmpfsOptionsFromMount(t *testing.T) { in: &config.Mount{Other: []string{"tmpfs-size=oops", "tmpfs-mode=oops"}}, want: nil, }, + { + name: "negative size dropped", + in: &config.Mount{Other: []string{"tmpfs-size=-1"}}, + want: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From dbf598785aa4c8bfc2f4c99efbadb27213effb2f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 16:51:59 -0500 Subject: [PATCH 08/13] feat(devcontainer): wire compose build --pull to ForceBuild The compose '--pull' flag was dead since the original compose-support commit: it was gated on 'extendImageBuildInfo == nil', but GetExtendedBuildInfo never returns nil on success, so --pull was never added. Replace the dead condition with a real signal: thread CLIOptions.ForceBuild through buildAndExtendParams.forceBuild and gate --pull on it, so a forced rebuild also re-pulls the base image. Wired at both call sites (compose up and standalone compose build). Currently reachable via the hidden --force-build flag; ready to attach to any future user-facing pull option. Adds TestComposeBuildArgs covering the pull flag, build verb, override file, and run-service de-duplication. --- pkg/devcontainer/build.go | 1 + pkg/devcontainer/compose.go | 4 ++ pkg/devcontainer/compose_build.go | 14 +++---- pkg/devcontainer/compose_test.go | 62 +++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 5a5b06d2e..a29eff71b 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -468,6 +468,7 @@ func (r *runner) buildDevImageCompose( composeService: &composeService, globalArgs: composeGlobalArgs, featureSecretsFile: options.FeatureSecretsFile, + forceBuild: options.ForceBuild, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index ce611cd81..b032d526d 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -73,6 +73,9 @@ type buildAndExtendParams struct { composeService *composetypes.ServiceConfig globalArgs []string featureSecretsFile string + // forceBuild re-pulls the base image during the compose build (--pull), + // set from CLIOptions.ForceBuild. + forceBuild bool } // composeUpParams groups the inputs shared by extendedDockerComposeUp and @@ -745,6 +748,7 @@ func (r *runner) buildComposeOverrideArgs( composeService: params.composeService, globalArgs: composeGlobalArgs, featureSecretsFile: start.options.FeatureSecretsFile, + forceBuild: start.options.ForceBuild, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 0bf232c8d..985c1c90c 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -145,13 +145,9 @@ func (r *runner) buildAndExtendDockerCompose( projectName: params.project.Name, globalArgs: params.globalArgs, overrideComposeFilePath: dockerComposeFilePath, - // extendImageBuildInfo is non-nil here (a nil result always comes with - // an error, handled above), so --pull is effectively never added. This - // preserves the original behavior; do not switch to hasFeatureBuildInfo - // without intending to start passing --pull on featureless builds. - pull: false, - serviceName: params.composeService.Name, - runServices: params.parsedConfig.Config.RunServices, + pull: params.forceBuild, + serviceName: params.composeService.Name, + runServices: params.parsedConfig.Config.RunServices, }) if err := r.runComposeBuild(ctx, params.composeHelper, buildArgs); err != nil { @@ -346,8 +342,8 @@ type composeBuildArgsParams struct { } // composeBuildArgs assembles the "docker compose ... build" argument list, -// adding the override file, --pull when params.pull is set, and any explicitly -// requested run services. +// adding the override file, --pull when a fresh base image is requested, and +// any explicitly requested run services. func composeBuildArgs(params *composeBuildArgsParams) []string { buildArgs := []string{composeProjectNameFlag, params.projectName} buildArgs = append(buildArgs, params.globalArgs...) diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index c950bcabc..2fa4dc48c 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -2,6 +2,7 @@ package devcontainer import ( "path/filepath" + "slices" "strings" "testing" @@ -667,3 +668,64 @@ func TestNamedVolumesFromMounts(t *testing.T) { } }) } + +func TestComposeBuildArgs(t *testing.T) { + t.Run("adds --pull when requested", func(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + pull: true, + }) + if !slices.Contains(args, "--pull") { + t.Errorf("expected --pull in %v", args) + } + }) + + t.Run("omits --pull by default", func(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + pull: false, + }) + if slices.Contains(args, "--pull") { + t.Errorf("did not expect --pull in %v", args) + } + }) + + t.Run("includes project name, build, and override file", func(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + overrideComposeFilePath: "/tmp/override.yml", + }) + if !slices.Contains(args, composeProjectNameFlag) || !slices.Contains(args, "ws") { + t.Errorf("expected project name flag in %v", args) + } + if !slices.Contains(args, "build") { + t.Errorf("expected build verb in %v", args) + } + if !slices.Contains(args, "/tmp/override.yml") { + t.Errorf("expected override file in %v", args) + } + }) + + t.Run("appends run services without duplicating the main service", func(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + runServices: []string{"app", "db"}, + }) + appCount := 0 + for _, a := range args { + if a == "app" { + appCount++ + } + } + if appCount != 1 { + t.Errorf("main service should appear once, got %d in %v", appCount, args) + } + if !slices.Contains(args, "db") { + t.Errorf("expected run service db in %v", args) + } + }) +} From 311049319642ca076e00e8981953532355e6e2c0 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 17:16:01 -0500 Subject: [PATCH 09/13] refactor(devcontainer): canonical build options + real --pull flag Redesigns the build-options topology that made the compose --pull flag dead, and turns --pull into a real user-facing option. Root cause: UpOptions had an outer ForceBuild that shadowed the embedded CLIOptions.ForceBuild and was never set, and the up path converted UpOptions -> BuildOptions with a lossy field-by-field copy that dropped most CLIOptions fields. Any per-build flag routed through this silently died. Changes: - Add Pull to provider.CLIOptions (json:"pull,omitempty"; wire-safe, backward-compatible, round-trips to the agent via EncodeOptions). - Remove the dead outer UpOptions.ForceBuild shadow. Keep NoBuild and RegistryCache (load-bearing: set by the tunnel / sourced from AgentWorkspaceInfo respectively) and document why. - Add UpOptions.toBuildOptions() that carries the full embedded CLIOptions so build flags are never dropped, replacing the lossy copy in buildNewContainerConfig. - Wire compose build --pull to CLIOptions.Pull at both build paths. - Expose --pull on both 'up' and 'build' commands. Incidental fix: the old lossy copy also dropped FeatureSecretsFile on the single-container up path, so feature secrets were silently ignored during 'up' builds (the build command was unaffected). toBuildOptions now carries it through. Repository/Tag are build-command-only and empty on up, so carrying them is a no-op. Adds TestUpOptionsToBuildOptions guarding against field-drop regressions. 311 tests pass. --- cmd/workspace/build.go | 2 ++ cmd/workspace/up/up_flags.go | 3 +++ pkg/devcontainer/build.go | 2 +- pkg/devcontainer/compose.go | 8 +++---- pkg/devcontainer/compose_build.go | 2 +- pkg/devcontainer/run.go | 18 ++++++++++++-- pkg/devcontainer/run_test.go | 39 +++++++++++++++++++++++++++++++ pkg/devcontainer/single.go | 18 +++++--------- pkg/provider/workspace.go | 3 +++ 9 files changed, 75 insertions(+), 20 deletions(-) diff --git a/cmd/workspace/build.go b/cmd/workspace/build.go index bb45a4e53..79b0f632b 100644 --- a/cmd/workspace/build.go +++ b/cmd/workspace/build.go @@ -100,6 +100,8 @@ func NewBuildCmd(flags *flags.GlobalFlags) *cobra.Command { StringVar(&cmd.ImageName, "image-name", "", "Alternative name for the built image") buildCmd.Flags(). BoolVar(&cmd.NoBuild, "no-build", false, "Fail if the image must be built (enforce pre-built images only)") + buildCmd.Flags(). + BoolVar(&cmd.Pull, "pull", false, "Always attempt to pull a newer version of the base image when building") // TESTING buildCmd.Flags().BoolVar(&cmd.ForceBuild, "force-build", false, "TESTING ONLY") diff --git a/cmd/workspace/up/up_flags.go b/cmd/workspace/up/up_flags.go index 042e92cef..ba3000357 100644 --- a/cmd/workspace/up/up_flags.go +++ b/cmd/workspace/up/up_flags.go @@ -195,6 +195,9 @@ func (cmd *UpCmd) registerWorkspaceFlags(upCmd *cobra.Command) { upCmd.Flags(). BoolVar(&cmd.Prebuild, "prebuild", false, "If true will only run the prebuild lifecycle (onCreateCommand + updateContentCommand) then stop") + upCmd.Flags(). + BoolVar(&cmd.Pull, "pull", false, + "Always attempt to pull a newer version of the base image when building") upCmd.Flags(). BoolVar(&cmd.Recreate, "recreate", false, "If true will remove any existing containers and recreate them") upCmd.Flags().BoolVar(&cmd.Recreate, "remove-existing-container", false, "Alias for --recreate") diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index a29eff71b..90a1ef688 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -468,7 +468,7 @@ func (r *runner) buildDevImageCompose( composeService: &composeService, globalArgs: composeGlobalArgs, featureSecretsFile: options.FeatureSecretsFile, - forceBuild: options.ForceBuild, + pull: options.Pull, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index b032d526d..87f558be3 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -73,9 +73,9 @@ type buildAndExtendParams struct { composeService *composetypes.ServiceConfig globalArgs []string featureSecretsFile string - // forceBuild re-pulls the base image during the compose build (--pull), - // set from CLIOptions.ForceBuild. - forceBuild bool + // pull re-pulls base images during the compose build (--pull), set from + // CLIOptions.Pull. + pull bool } // composeUpParams groups the inputs shared by extendedDockerComposeUp and @@ -748,7 +748,7 @@ func (r *runner) buildComposeOverrideArgs( composeService: params.composeService, globalArgs: composeGlobalArgs, featureSecretsFile: start.options.FeatureSecretsFile, - forceBuild: start.options.ForceBuild, + pull: start.options.Pull, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 985c1c90c..3aba2178a 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -145,7 +145,7 @@ func (r *runner) buildAndExtendDockerCompose( projectName: params.project.Name, globalArgs: params.globalArgs, overrideComposeFilePath: dockerComposeFilePath, - pull: params.forceBuild, + pull: params.pull, serviceName: params.composeService.Name, runServices: params.parsedConfig.Config.RunServices, }) diff --git a/pkg/devcontainer/run.go b/pkg/devcontainer/run.go index 2b4af6629..d13287a54 100644 --- a/pkg/devcontainer/run.go +++ b/pkg/devcontainer/run.go @@ -89,11 +89,25 @@ type runner struct { type UpOptions struct { provider2.CLIOptions - NoBuild bool - ForceBuild bool + // NoBuild is set by the container tunnel to force pre-built images; it is + // distinct from the embedded CLIOptions.NoBuild used by the build command. + NoBuild bool + // RegistryCache is sourced from AgentWorkspaceInfo.RegistryCache (a provider + // context option), not from CLIOptions, so it is a separate field. RegistryCache string } +// toBuildOptions derives the BuildOptions for an up-triggered build. It carries +// the full embedded CLIOptions (so build flags like Pull/ForceBuild are never +// dropped) and applies the up-specific NoBuild/RegistryCache overrides. +func (o UpOptions) toBuildOptions() provider2.BuildOptions { + return provider2.BuildOptions{ + CLIOptions: o.CLIOptions, + RegistryCache: o.RegistryCache, + NoBuild: o.NoBuild, + } +} + // runContainerParams groups the inputs shared by the runSingleContainer, // runDockerCompose, and runDefaultContainer dispatch methods. type runContainerParams struct { diff --git a/pkg/devcontainer/run_test.go b/pkg/devcontainer/run_test.go index 444673a28..76ed9038c 100644 --- a/pkg/devcontainer/run_test.go +++ b/pkg/devcontainer/run_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/devsy-org/devsy/pkg/devcontainer/config" + provider2 "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/types" ) @@ -294,3 +295,41 @@ func TestMountSetConsistency(t *testing.T) { }) } } + +func TestUpOptionsToBuildOptions(t *testing.T) { + up := UpOptions{ + CLIOptions: provider2.CLIOptions{ + Pull: true, + ForceBuild: true, + ForceDockerless: true, + ExtraDevContainerPath: "/extra.json", + }, + NoBuild: true, + RegistryCache: "registry.example.com/cache", + } + + got := up.toBuildOptions() + + // Build flags carried via the embedded CLIOptions must survive the + // conversion (regression guard against the old lossy field-by-field copy). + if !got.Pull { + t.Error("Pull should be carried through to BuildOptions") + } + if !got.ForceBuild { + t.Error("ForceBuild should be carried through to BuildOptions") + } + if !got.ForceDockerless { + t.Error("ForceDockerless should be carried through to BuildOptions") + } + if got.ExtraDevContainerPath != "/extra.json" { + t.Errorf("ExtraDevContainerPath = %q, want /extra.json", got.ExtraDevContainerPath) + } + + // Up-specific overrides. + if !got.NoBuild { + t.Error("NoBuild override should be applied") + } + if got.RegistryCache != "registry.example.com/cache" { + t.Errorf("RegistryCache = %q, want registry.example.com/cache", got.RegistryCache) + } +} diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 28ae12e1c..4dfe350f5 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -15,7 +15,6 @@ import ( "github.com/devsy-org/devsy/pkg/devcontainer/metadata" "github.com/devsy-org/devsy/pkg/driver" "github.com/devsy-org/devsy/pkg/log" - provider2 "github.com/devsy-org/devsy/pkg/provider" ) var dockerlessImage = "ghcr.io/devsy-org/dockerless:0.2.0" @@ -301,17 +300,12 @@ func (r *runner) buildNewContainerConfig( ctx context.Context, p *resolveParams, ) (*config.BuildInfo, *config.MergedDevContainerConfig, error) { - buildInfo, err := r.build(ctx, p.parsedConfig, p.substitutionContext, provider2.BuildOptions{ - CLIOptions: provider2.CLIOptions{ - PrebuildRepositories: p.options.PrebuildRepositories, - ForceDockerless: p.options.ForceDockerless, - Platform: p.options.Platform, - ExtraDevContainerPath: p.options.ExtraDevContainerPath, - }, - NoBuild: p.options.NoBuild, - RegistryCache: p.options.RegistryCache, - ExportCache: false, - }) + buildInfo, err := r.build( + ctx, + p.parsedConfig, + p.substitutionContext, + p.options.toBuildOptions(), + ) if err != nil { return nil, nil, fmt.Errorf("build image: %w", err) } diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index ba3b3d240..d3862f423 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -296,6 +296,9 @@ type CLIOptions struct { // ForceBuild forces a rebuild even if a cached image exists. ForceBuild bool `json:"forceBuild,omitempty"` + // Pull re-pulls base images during the build (docker build --pull) instead + // of reusing locally cached layers. + Pull bool `json:"pull,omitempty"` // ForceDockerless forces the use of a dockerless build approach. ForceDockerless bool `json:"forceDockerless,omitempty"` // ForceInternalBuildKit forces the use of internal BuildKit instead of docker buildx. From 684b5bdfc64fc69f95658a3a0e1d6ff84dc1ba21 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 17:29:13 -0500 Subject: [PATCH 10/13] feat(devcontainer): honor --pull and --no-cache on both build paths --pull and --no-cache were each wired to only one of the two build executors: --no-cache worked for single-container/dockerfile builds but not compose, and --pull (added earlier) worked for compose but not single-container. Close both mirror gaps so both modifiers are path-agnostic: - single-container: add Pull to build.BuildOptions and emit --pull in buildDockerBuildxArgs (alongside the existing --no-cache). - compose: add noCache to composeBuildArgs and wire it from CLIOptions.NoCache at both compose call sites. - expose --no-cache on the up command (already on build); --pull is now on both as well. ForceBuild is intentionally left separate: in devsy it means 'skip prebuild-image reuse', which is orthogonal to pull/no-cache and is not folded into them. Internal-buildkit path is unchanged (it already ignores --no-cache too; out of scope). Adds buildx-args and compose-args tests for both modifiers. 985 tests pass. --- cmd/workspace/up/up_flags.go | 3 ++ pkg/devcontainer/build.go | 1 + pkg/devcontainer/build/options.go | 5 +++- pkg/devcontainer/compose.go | 4 +++ pkg/devcontainer/compose_build.go | 9 ++++-- pkg/devcontainer/compose_test.go | 49 ++++++++++++++++++------------- pkg/driver/docker/build.go | 3 ++ pkg/driver/docker/build_test.go | 28 ++++++++++++++++++ 8 files changed, 79 insertions(+), 23 deletions(-) diff --git a/cmd/workspace/up/up_flags.go b/cmd/workspace/up/up_flags.go index ba3000357..7d1c4f9ee 100644 --- a/cmd/workspace/up/up_flags.go +++ b/cmd/workspace/up/up_flags.go @@ -198,6 +198,9 @@ func (cmd *UpCmd) registerWorkspaceFlags(upCmd *cobra.Command) { upCmd.Flags(). BoolVar(&cmd.Pull, "pull", false, "Always attempt to pull a newer version of the base image when building") + upCmd.Flags(). + BoolVar(&cmd.NoCache, "no-cache", false, + "Do not use the build cache when building the image") upCmd.Flags(). BoolVar(&cmd.Recreate, "recreate", false, "If true will remove any existing containers and recreate them") upCmd.Flags().BoolVar(&cmd.Recreate, "remove-existing-container", false, "Alias for --recreate") diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 90a1ef688..dfc32d68d 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -469,6 +469,7 @@ func (r *runner) buildDevImageCompose( globalArgs: composeGlobalArgs, featureSecretsFile: options.FeatureSecretsFile, pull: options.Pull, + noCache: options.NoCache, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/build/options.go b/pkg/devcontainer/build/options.go index 39eb5c1e3..2eb2c87f7 100644 --- a/pkg/devcontainer/build/options.go +++ b/pkg/devcontainer/build/options.go @@ -52,8 +52,10 @@ type BuildOptions struct { Push bool // Upload controls whether to upload the build context. Used for remote builds. Upload bool - // NoCache disables the Docker build cache entirely. + // NoCache disables the Docker build cache entirely (--no-cache). NoCache bool + // Pull always attempts to pull a newer version of base images (--pull). + Pull bool } // NewOptionsParams contains the parameters needed to create BuildOptions. @@ -96,6 +98,7 @@ func NewOptions(params NewOptionsParams) (*BuildOptions, error) { // directly to the registry. This is mutually exclusive with Load. Push: params.Options.PushDuringBuild, NoCache: params.Options.NoCache, + Pull: params.Options.Pull, } // get build args and target diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index 87f558be3..cbcae5484 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -76,6 +76,9 @@ type buildAndExtendParams struct { // pull re-pulls base images during the compose build (--pull), set from // CLIOptions.Pull. pull bool + // noCache disables the build cache during the compose build (--no-cache), + // set from CLIOptions.NoCache. + noCache bool } // composeUpParams groups the inputs shared by extendedDockerComposeUp and @@ -749,6 +752,7 @@ func (r *runner) buildComposeOverrideArgs( globalArgs: composeGlobalArgs, featureSecretsFile: start.options.FeatureSecretsFile, pull: start.options.Pull, + noCache: start.options.NoCache, }) if err != nil { return nil, fmt.Errorf("build and extend docker-compose: %w", err) diff --git a/pkg/devcontainer/compose_build.go b/pkg/devcontainer/compose_build.go index 3aba2178a..575a1f1fd 100644 --- a/pkg/devcontainer/compose_build.go +++ b/pkg/devcontainer/compose_build.go @@ -146,6 +146,7 @@ func (r *runner) buildAndExtendDockerCompose( globalArgs: params.globalArgs, overrideComposeFilePath: dockerComposeFilePath, pull: params.pull, + noCache: params.noCache, serviceName: params.composeService.Name, runServices: params.parsedConfig.Config.RunServices, }) @@ -337,13 +338,14 @@ type composeBuildArgsParams struct { globalArgs []string overrideComposeFilePath string pull bool + noCache bool serviceName string runServices []string } // composeBuildArgs assembles the "docker compose ... build" argument list, -// adding the override file, --pull when a fresh base image is requested, and -// any explicitly requested run services. +// adding the override file, --pull/--no-cache build modifiers, and any +// explicitly requested run services. func composeBuildArgs(params *composeBuildArgsParams) []string { buildArgs := []string{composeProjectNameFlag, params.projectName} buildArgs = append(buildArgs, params.globalArgs...) @@ -354,6 +356,9 @@ func composeBuildArgs(params *composeBuildArgsParams) []string { if params.pull { buildArgs = append(buildArgs, "--pull") } + if params.noCache { + buildArgs = append(buildArgs, "--no-cache") + } // Only run the services defined in .devcontainer.json runServices if len(params.runServices) > 0 { diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 2fa4dc48c..7f645990f 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -670,28 +670,37 @@ func TestNamedVolumesFromMounts(t *testing.T) { } func TestComposeBuildArgs(t *testing.T) { - t.Run("adds --pull when requested", func(t *testing.T) { - args := composeBuildArgs(&composeBuildArgsParams{ - projectName: "ws", - serviceName: "app", - pull: true, - }) - if !slices.Contains(args, "--pull") { - t.Errorf("expected --pull in %v", args) - } - }) - - t.Run("omits --pull by default", func(t *testing.T) { - args := composeBuildArgs(&composeBuildArgsParams{ - projectName: "ws", - serviceName: "app", - pull: false, + modifierTests := []struct { + name string + pull bool + noCache bool + wantPull bool + wantNoCache bool + }{ + {name: "no modifiers"}, + {name: "pull only", pull: true, wantPull: true}, + {name: "no-cache only", noCache: true, wantNoCache: true}, + {name: "both", pull: true, noCache: true, wantPull: true, wantNoCache: true}, + } + for _, tt := range modifierTests { + t.Run(tt.name, func(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + pull: tt.pull, + noCache: tt.noCache, + }) + if got := slices.Contains(args, "--pull"); got != tt.wantPull { + t.Errorf("--pull present = %v, want %v (args=%v)", got, tt.wantPull, args) + } + if got := slices.Contains(args, "--no-cache"); got != tt.wantNoCache { + t.Errorf("--no-cache present = %v, want %v (args=%v)", got, tt.wantNoCache, args) + } }) - if slices.Contains(args, "--pull") { - t.Errorf("did not expect --pull in %v", args) - } - }) + } +} +func TestComposeBuildArgsStructure(t *testing.T) { t.Run("includes project name, build, and override file", func(t *testing.T) { args := composeBuildArgs(&composeBuildArgsParams{ projectName: "ws", diff --git a/pkg/driver/docker/build.go b/pkg/driver/docker/build.go index 9df78524e..184f5f559 100644 --- a/pkg/driver/docker/build.go +++ b/pkg/driver/docker/build.go @@ -101,6 +101,9 @@ func buildDockerBuildxArgs(options *build.BuildOptions, platform string) []strin if options.NoCache { args = append(args, "--no-cache") } + if options.Pull { + args = append(args, "--pull") + } args = appendImageTags(args, options.Images) args = appendBuildArgsAndContexts(args, options.BuildArgs, options.Contexts) args = appendLabels(args, options.Labels) diff --git a/pkg/driver/docker/build_test.go b/pkg/driver/docker/build_test.go index 896b9e728..a6442b937 100644 --- a/pkg/driver/docker/build_test.go +++ b/pkg/driver/docker/build_test.go @@ -4,8 +4,10 @@ import ( "os" "os/exec" "path/filepath" + "slices" "testing" + "github.com/devsy-org/devsy/pkg/devcontainer/build" "github.com/devsy-org/devsy/pkg/docker" "github.com/devsy-org/devsy/pkg/provider" "github.com/stretchr/testify/assert" @@ -109,3 +111,29 @@ func TestSelectStrategy_DockerRuntime_BuildxWhenAvailable(t *testing.T) { t.Fatalf("unexpected strategy type: %T", strategy) } } + +func TestBuildDockerBuildxArgs_PullAndNoCache(t *testing.T) { + tests := []struct { + name string + opts *build.BuildOptions + wantPull bool + wantNoCach bool + }{ + {name: "neither", opts: &build.BuildOptions{}}, + {name: "pull only", opts: &build.BuildOptions{Pull: true}, wantPull: true}, + {name: "no-cache only", opts: &build.BuildOptions{NoCache: true}, wantNoCach: true}, + { + name: "both", + opts: &build.BuildOptions{Pull: true, NoCache: true}, + wantPull: true, + wantNoCach: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + args := buildDockerBuildxArgs(tt.opts, "") + assert.Equal(t, tt.wantPull, slices.Contains(args, "--pull"), "args=%v", args) + assert.Equal(t, tt.wantNoCach, slices.Contains(args, "--no-cache"), "args=%v", args) + }) + } +} From 252ad841c9f140dd8c6c7edaa20774c1b1bcfe9b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 17:42:02 -0500 Subject: [PATCH 11/13] refactor(devcontainer): decompose build.go and clear all lint issues Resolves all 9 lint findings on build.go (argument-limit x2, cyclop x3, funlen, nestif, gosec x2, gocritic) with no behavior change. - build(): if-else chain -> switch (gocritic). - buildImage/dockerlessFallback: 8- and 7-arg signatures -> buildImageParams and dockerlessFallbackParams (argument-limit). - Extract focused helpers to cut complexity: resolveDockerfileBuildBase, validateDockerfileTarget, resolveDockerfileUser, findPrebuildImage (also clears the prebuild-loop nestif), executeBuild (build-backend dispatch), composeBuildInfo. - buildDevImageCompose reuses dockerComposeProjectFiles + loadComposeProject instead of re-assembling compose args inline. - gosec: annotate the trusted-config Dockerfile read (#nosec G304); the G703 taint finding clears once its source read is annotated. build.go: 9 -> 0 lint issues (verified uncapped, incl. gosec-only run). Package total 40 -> 31, nothing pushed to other files. 985 tests pass. --- pkg/devcontainer/build.go | 424 +++++++++++++++++++++++--------------- 1 file changed, 262 insertions(+), 162 deletions(-) diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index dfc32d68d..83504c2df 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -8,7 +8,6 @@ import ( "path/filepath" "strings" - "github.com/devsy-org/devsy/pkg/compose" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/build" "github.com/devsy-org/devsy/pkg/devcontainer/buildkit" @@ -31,11 +30,12 @@ func (r *runner) build( var buildInfo *config.BuildInfo var err error - if isDockerFileConfig(parsedConfig.Config) { + switch { + case isDockerFileConfig(parsedConfig.Config): buildInfo, err = r.buildAndExtendImage(ctx, parsedConfig, substitutionContext, options) - } else if isDockerComposeConfig(parsedConfig.Config) { + case isDockerComposeConfig(parsedConfig.Config): buildInfo, err = r.buildDevImageCompose(ctx, parsedConfig, substitutionContext, options) - } else { + default: buildInfo, err = r.extendImage(ctx, parsedConfig, substitutionContext, options) } @@ -95,56 +95,80 @@ func (r *runner) extendImage( } // build the image - return r.buildImage( - ctx, - parsedConfig, - substitutionContext, - imageBuildInfo, - extendedBuildInfo, - "", - "", - options, - ) + return r.buildImage(ctx, &buildImageParams{ + parsedConfig: parsedConfig, + substitutionContext: substitutionContext, + buildInfo: imageBuildInfo, + extendedBuildInfo: extendedBuildInfo, + options: options, + }) } -func (r *runner) buildAndExtendImage( - ctx context.Context, +// dockerfileBuildBase holds the resolved Dockerfile and the image base/target +// stage for a dockerfile-backed build. +type dockerfileBuildBase struct { + path string + content []byte + imageBase string +} + +// resolveDockerfileBuildBase locates and reads the Dockerfile and determines the +// image base/target stage, ensuring a final stage name when no target is set. +func (r *runner) resolveDockerfileBuildBase( parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - options provider.BuildOptions, -) (*config.BuildInfo, error) { +) (*dockerfileBuildBase, error) { dockerFilePath, err := r.getDockerfilePath(parsedConfig.Config) if err != nil { return nil, err } + // #nosec G304 -- dockerFilePath is derived from trusted devcontainer config. dockerFileContent, err := os.ReadFile(dockerFilePath) if err != nil { return nil, err } - // ensure there is a target to choose for us - var imageBase string - if parsedConfig.Config.GetTarget() != "" { - imageBase = parsedConfig.Config.GetTarget() - } else { - lastTargetName, modifiedDockerfileContents, err := dockerfile.EnsureFinalStageName( - string(dockerFileContent), - config.DockerfileDefaultTarget, - ) - if err != nil { - return nil, err - } else if modifiedDockerfileContents != "" { - dockerFileContent = []byte(modifiedDockerfileContents) - } + if target := parsedConfig.Config.GetTarget(); target != "" { + return &dockerfileBuildBase{ + path: dockerFilePath, + content: dockerFileContent, + imageBase: target, + }, nil + } + + lastTargetName, modifiedDockerfileContents, err := dockerfile.EnsureFinalStageName( + string(dockerFileContent), + config.DockerfileDefaultTarget, + ) + if err != nil { + return nil, err + } + if modifiedDockerfileContents != "" { + dockerFileContent = []byte(modifiedDockerfileContents) + } + + return &dockerfileBuildBase{ + path: dockerFilePath, + content: dockerFileContent, + imageBase: lastTargetName, + }, nil +} - imageBase = lastTargetName +func (r *runner) buildAndExtendImage( + ctx context.Context, + parsedConfig *config.SubstitutedConfig, + substitutionContext *config.SubstitutionContext, + options provider.BuildOptions, +) (*config.BuildInfo, error) { + base, err := r.resolveDockerfileBuildBase(parsedConfig) + if err != nil { + return nil, err } // get image build info imageBuildInfo, err := r.getImageBuildInfoFromDockerfile( substitutionContext, - string(dockerFileContent), + string(base.content), parsedConfig.Config.GetArgs(), parsedConfig.Config.GetTarget(), ) @@ -156,7 +180,7 @@ func (r *runner) buildAndExtendImage( extendedBuildInfo, err := feature.GetExtendedBuildInfo(&feature.ExtendedBuildParams{ Ctx: substitutionContext, ImageBuildInfo: imageBuildInfo, - Target: imageBase, + Target: base.imageBase, DevContainerConfig: parsedConfig, ForceBuild: options.ForceBuild, SecretOpts: featureSecretOpts(options), @@ -166,16 +190,15 @@ func (r *runner) buildAndExtendImage( } // build the image - return r.buildImage( - ctx, - parsedConfig, - substitutionContext, - imageBuildInfo, - extendedBuildInfo, - dockerFilePath, - string(dockerFileContent), - options, - ) + return r.buildImage(ctx, &buildImageParams{ + parsedConfig: parsedConfig, + substitutionContext: substitutionContext, + buildInfo: imageBuildInfo, + extendedBuildInfo: extendedBuildInfo, + dockerfilePath: base.path, + dockerfileContent: string(base.content), + options: options, + }) } func (r *runner) getDockerfilePath(parsedConfig *config.DevContainerConfig) (string, error) { @@ -252,12 +275,8 @@ func (r *runner) getImageBuildInfoFromDockerfile( return nil, fmt.Errorf("parse dockerfile: %w", err) } - // Check that the build target specified in the devcontainer.json exists in the Dockerfile - if target != "" && parsedDockerfile.StagesByTarget != nil { - _, ok := parsedDockerfile.StagesByTarget[target] - if !ok { - return nil, fmt.Errorf("build target does not exist") - } + if err := validateDockerfileTarget(parsedDockerfile, target); err != nil { + return nil, err } baseImage := parsedDockerfile.FindBaseImage(buildArgs, target) @@ -270,8 +289,42 @@ func (r *runner) getImageBuildInfoFromDockerfile( return nil, fmt.Errorf("inspect image %s: %w", baseImage, err) } - // find user - user := parsedDockerfile.FindUserStatement( + user := resolveDockerfileUser(parsedDockerfile, buildArgs, imageDetails, target) + + // parse metadata from image details + imageMetadataConfig, err := metadata.GetImageMetadata(imageDetails, substitutionContext) + if err != nil { + return nil, fmt.Errorf("get image metadata: %w", err) + } + + return &config.ImageBuildInfo{ + Dockerfile: parsedDockerfile, + User: user, + Metadata: imageMetadataConfig, + }, nil +} + +// validateDockerfileTarget ensures a non-empty build target exists in the +// parsed Dockerfile's stages. +func validateDockerfileTarget(parsed *dockerfile.Dockerfile, target string) error { + if target == "" || parsed.StagesByTarget == nil { + return nil + } + if _, ok := parsed.StagesByTarget[target]; !ok { + return fmt.Errorf("build target does not exist") + } + return nil +} + +// resolveDockerfileUser determines the build user, preferring a USER statement +// in the Dockerfile, then the base image's user, then "root". +func resolveDockerfileUser( + parsed *dockerfile.Dockerfile, + buildArgs map[string]string, + imageDetails *config.ImageDetails, + target string, +) string { + user := parsed.FindUserStatement( buildArgs, config.ListToObject(imageDetails.Config.Env), target, @@ -282,30 +335,87 @@ func (r *runner) getImageBuildInfoFromDockerfile( if user == "" { user = "root" } + return user +} - // parse metadata from image details - imageMetadataConfig, err := metadata.GetImageMetadata(imageDetails, substitutionContext) - if err != nil { - return nil, fmt.Errorf("get image metadata: %w", err) +// prebuildLookupParams groups the inputs for locating an existing prebuild image. +type prebuildLookupParams struct { + parsedConfig *config.SubstitutedConfig + extendedBuildInfo *feature.ExtendedBuildInfo + options provider.BuildOptions + prebuildHash string + targetArch string +} + +// findPrebuildImage searches the configured prebuild repositories for an image +// matching the prebuild hash and target architecture. It returns the resolved +// BuildInfo when found, or (nil, nil) when no prebuild image is available. +func (r *runner) findPrebuildImage( + ctx context.Context, + params *prebuildLookupParams, +) (*config.BuildInfo, error) { + options := params.options + devsyCustomizations := config.GetDevsyCustomizations(params.parsedConfig.Config) + if options.Repository != "" { + options.PrebuildRepositories = append(options.PrebuildRepositories, options.Repository) + } + options.PrebuildRepositories = append( + options.PrebuildRepositories, + devsyCustomizations.PrebuildRepository...) + + log.Debugf( + "Try to find prebuild image %s in repositories %s", + params.prebuildHash, + strings.Join(options.PrebuildRepositories, ","), + ) + for _, prebuildRepo := range options.PrebuildRepositories { + prebuildImage := prebuildRepo + ":" + params.prebuildHash + img, err := image.GetImageForArch(ctx, prebuildImage, params.targetArch) + if err != nil { + log.Debugf("Error trying to find prebuild image %s: %v", prebuildImage, err) + continue + } + if img == nil { + continue + } + + log.Infof("Found existing prebuilt image %s", prebuildImage) + imageDetails, err := r.inspectImage(ctx, prebuildImage) + if err != nil { + return nil, fmt.Errorf("get image details: %w", err) + } + + return &config.BuildInfo{ + ImageDetails: imageDetails, + ImageMetadata: params.extendedBuildInfo.MetadataConfig, + ImageName: prebuildImage, + PrebuildHash: params.prebuildHash, + RegistryCache: options.RegistryCache, + Tags: options.Tag, + }, nil } - return &config.ImageBuildInfo{ - Dockerfile: parsedDockerfile, - User: user, - Metadata: imageMetadataConfig, - }, nil + return nil, nil +} + +// buildImageParams groups the inputs for building a dockerfile-backed image. +type buildImageParams struct { + parsedConfig *config.SubstitutedConfig + substitutionContext *config.SubstitutionContext + buildInfo *config.ImageBuildInfo + extendedBuildInfo *feature.ExtendedBuildInfo + dockerfilePath string + dockerfileContent string + options provider.BuildOptions } func (r *runner) buildImage( ctx context.Context, - parsedConfig *config.SubstitutedConfig, - substitutionContext *config.SubstitutionContext, - buildInfo *config.ImageBuildInfo, - extendedBuildInfo *feature.ExtendedBuildInfo, - dockerfilePath, - dockerfileContent string, - options provider.BuildOptions, + params *buildImageParams, ) (*config.BuildInfo, error) { + parsedConfig := params.parsedConfig + options := params.options + targetArch, err := r.Driver.TargetArchitecture(ctx, r.ID) if err != nil { return nil, err @@ -316,9 +426,9 @@ func (r *runner) buildImage( Platform: options.Platform, Architecture: targetArch, ContextPath: config.GetContextPath(parsedConfig.Config), - DockerfilePath: dockerfilePath, - DockerfileContent: dockerfileContent, - BuildInfo: buildInfo, + DockerfilePath: params.dockerfilePath, + DockerfileContent: params.dockerfileContent, + BuildInfo: params.buildInfo, }) if err != nil { return nil, err @@ -326,53 +436,41 @@ func (r *runner) buildImage( // check if there is a prebuild image if !options.ForceDockerless && !options.ForceBuild { - devsyCustomizations := config.GetDevsyCustomizations(parsedConfig.Config) - if options.Repository != "" { - options.PrebuildRepositories = append(options.PrebuildRepositories, options.Repository) + prebuilt, err := r.findPrebuildImage(ctx, &prebuildLookupParams{ + parsedConfig: parsedConfig, + extendedBuildInfo: params.extendedBuildInfo, + options: options, + prebuildHash: prebuildHash, + targetArch: targetArch, + }) + if err != nil { + return nil, err } - options.PrebuildRepositories = append( - options.PrebuildRepositories, - devsyCustomizations.PrebuildRepository...) - - log.Debugf( - "Try to find prebuild image %s in repositories %s", - prebuildHash, - strings.Join(options.PrebuildRepositories, ","), - ) - for _, prebuildRepo := range options.PrebuildRepositories { - prebuildImage := prebuildRepo + ":" + prebuildHash - img, err := image.GetImageForArch(ctx, prebuildImage, targetArch) - if err == nil && img != nil { - // prebuild image found - log.Infof("Found existing prebuilt image %s", prebuildImage) - - // inspect image - imageDetails, err := r.inspectImage(ctx, prebuildImage) - if err != nil { - return nil, fmt.Errorf("get image details: %w", err) - } - - return &config.BuildInfo{ - ImageDetails: imageDetails, - ImageMetadata: extendedBuildInfo.MetadataConfig, - ImageName: prebuildImage, - PrebuildHash: prebuildHash, - RegistryCache: options.RegistryCache, - Tags: options.Tag, - }, nil - } else if err != nil { - log.Debugf("Error trying to find prebuild image %s: %v", prebuildImage, err) - } + if prebuilt != nil { + return prebuilt, nil } } + return r.executeBuild(ctx, params, prebuildHash, targetArch) +} + +// executeBuild dispatches the actual image build to the appropriate backend: +// remote BuildKit (platform mode), the dockerless fallback (non-docker driver), +// or the docker driver. +func (r *runner) executeBuild( + ctx context.Context, + params *buildImageParams, + prebuildHash, targetArch string, +) (*config.BuildInfo, error) { + options := params.options + if options.CLIOptions.Platform.Enabled { buildInfo, err := buildkit.BuildRemote(ctx, buildkit.BuildRemoteOptions{ PrebuildHash: prebuildHash, - ParsedConfig: parsedConfig, - ExtendedBuildInfo: extendedBuildInfo, - DockerfilePath: dockerfilePath, - DockerfileContent: dockerfileContent, + ParsedConfig: params.parsedConfig, + ExtendedBuildInfo: params.extendedBuildInfo, + DockerfilePath: params.dockerfilePath, + DockerfileContent: params.dockerfileContent, LocalWorkspaceFolder: r.LocalWorkspaceFolder, Options: options, TargetArch: targetArch, @@ -394,23 +492,23 @@ func (r *runner) buildImage( ) } - return dockerlessFallback( - r.LocalWorkspaceFolder, - substitutionContext.ContainerWorkspaceFolder, - parsedConfig, - buildInfo, - extendedBuildInfo, - dockerfileContent, - options, - ) + return dockerlessFallback(&dockerlessFallbackParams{ + localWorkspaceFolder: r.LocalWorkspaceFolder, + containerWorkspaceFolder: params.substitutionContext.ContainerWorkspaceFolder, + parsedConfig: params.parsedConfig, + buildInfo: params.buildInfo, + extendedBuildInfo: params.extendedBuildInfo, + dockerfileContent: params.dockerfileContent, + options: options, + }) } return dockerDriver.BuildDevContainer(ctx, driver.BuildRequest{ PrebuildHash: prebuildHash, - ParsedConfig: parsedConfig, - ExtendedBuildInfo: extendedBuildInfo, - DockerfilePath: dockerfilePath, - DockerfileContent: dockerfileContent, + ParsedConfig: params.parsedConfig, + ExtendedBuildInfo: params.extendedBuildInfo, + DockerfilePath: params.dockerfilePath, + DockerfileContent: params.dockerfileContent, LocalWorkspaceFolder: r.LocalWorkspaceFolder, Options: options, }) @@ -427,29 +525,15 @@ func (r *runner) buildDevImageCompose( return nil, fmt.Errorf("find docker compose: %w", err) } - envFiles := r.getEnvFiles() - - composeFiles, err := r.getDockerComposeFilePaths(parsedConfig, envFiles) + projFiles, err := r.dockerComposeProjectFiles(parsedConfig) if err != nil { - return nil, fmt.Errorf("get docker compose file paths: %w", err) - } - - var composeGlobalArgs []string - for _, configFile := range composeFiles { - composeGlobalArgs = append(composeGlobalArgs, "-f", configFile) - } - - for _, envFile := range envFiles { - composeGlobalArgs = append(composeGlobalArgs, "--env-file", envFile) + return nil, err } - log.Debugf("Loading docker compose project %+v", composeFiles) - project, err := compose.LoadDockerComposeProject(ctx, composeFiles, envFiles) + project, err := r.loadComposeProject(ctx, composeHelper, parsedConfig, projFiles) if err != nil { - return nil, fmt.Errorf("load docker compose project: %w", err) + return nil, err } - project.Name = composeHelper.GetProjectName(r.ID) - log.Debugf("Loaded project %s", project.Name) composeService, originalImageName, err := resolveComposeServiceImage( project, @@ -466,7 +550,7 @@ func (r *runner) buildDevImageCompose( project: project, composeHelper: composeHelper, composeService: &composeService, - globalArgs: composeGlobalArgs, + globalArgs: projFiles.composeGlobalArgs, featureSecretsFile: options.FeatureSecretsFile, pull: options.Pull, noCache: options.NoCache, @@ -475,6 +559,18 @@ func (r *runner) buildDevImageCompose( return nil, fmt.Errorf("build and extend docker-compose: %w", err) } + return r.composeBuildInfo(ctx, extendResult, originalImageName, options) +} + +// composeBuildInfo resolves the final image for a compose build and assembles +// the resulting BuildInfo. Compose builds do not compute a prebuild hash, so the +// image tag is used as the PrebuildHash fallback. +func (r *runner) composeBuildInfo( + ctx context.Context, + extendResult composeExtendResult, + originalImageName string, + options provider.BuildOptions, +) (*config.BuildInfo, error) { currentImageName := extendResult.buildImageName if currentImageName == "" { currentImageName = originalImageName @@ -485,9 +581,6 @@ func (r *runner) buildDevImageCompose( return nil, fmt.Errorf("inspect image: %w", err) } - // have a fallback value for PrebuildHash - // we don't calculate prebuild hash on docker compose builds - // let's use Images :tag then imageTag, err := r.getImageTag(ctx, imageDetails.ID) if err != nil { return nil, fmt.Errorf("inspect image: %w", err) @@ -503,15 +596,22 @@ func (r *runner) buildDevImageCompose( }, nil } -func dockerlessFallback( - localWorkspaceFolder, - containerWorkspaceFolder string, - parsedConfig *config.SubstitutedConfig, - buildInfo *config.ImageBuildInfo, - extendedBuildInfo *feature.ExtendedBuildInfo, - dockerfileContent string, - options provider.BuildOptions, -) (*config.BuildInfo, error) { +// dockerlessFallbackParams groups the inputs for the dockerless build fallback. +type dockerlessFallbackParams struct { + localWorkspaceFolder string + containerWorkspaceFolder string + parsedConfig *config.SubstitutedConfig + buildInfo *config.ImageBuildInfo + extendedBuildInfo *feature.ExtendedBuildInfo + dockerfileContent string + options provider.BuildOptions +} + +func dockerlessFallback(params *dockerlessFallbackParams) (*config.BuildInfo, error) { + parsedConfig := params.parsedConfig + extendedBuildInfo := params.extendedBuildInfo + options := params.options + contextPath := config.GetContextPath(parsedConfig.Config) devsyInternalFolder := filepath.Join(contextPath, config.DevsyContextFeatureFolder) // #nosec G301 -- TODO Consider using a more secure permission setting and ownership if needed. @@ -521,12 +621,12 @@ func dockerlessFallback( } // build dockerfile - devsyDockerfile, err := build.RewriteDockerfile(dockerfileContent, extendedBuildInfo) + devsyDockerfile, err := build.RewriteDockerfile(params.dockerfileContent, extendedBuildInfo) if err != nil { return nil, fmt.Errorf("rewrite dockerfile: %w", err) } else if devsyDockerfile == "" { devsyDockerfile = filepath.Join(devsyInternalFolder, "Dockerfile-without-features") - err = os.WriteFile(devsyDockerfile, []byte(dockerfileContent), 0o600) + err = os.WriteFile(devsyDockerfile, []byte(params.dockerfileContent), 0o600) if err != nil { return nil, fmt.Errorf("write devsy dockerfile: %w", err) } @@ -534,8 +634,8 @@ func dockerlessFallback( // get build args and target containerContext, containerDockerfile := getContainerContextAndDockerfile( - localWorkspaceFolder, - containerWorkspaceFolder, + params.localWorkspaceFolder, + params.containerWorkspaceFolder, contextPath, devsyDockerfile, ) @@ -549,7 +649,7 @@ func dockerlessFallback( BuildArgs: buildArgs, Target: target, - User: buildInfo.User, + User: params.buildInfo.User, }, RegistryCache: options.RegistryCache, Tags: options.Tag, From 28d81e485259ad433a6181160b40f82e3d9f0ee6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Sun, 28 Jun 2026 17:58:29 -0500 Subject: [PATCH 12/13] test(devcontainer): split TestComposeBuildArgsStructure to satisfy cyclop CI lint (--new-from-patch) flagged the new TestComposeBuildArgsStructure at cyclomatic complexity 9 (max 8). Split its two subtests into TestComposeBuildArgsIncludesCoreArgs and TestComposeBuildArgsRunServicesNoDuplicate, each well under threshold. Verified clean with --new-from-merge-base=origin/main. --- pkg/devcontainer/compose_test.go | 62 +++++++++++++++----------------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 7f645990f..ea0c08516 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -700,41 +700,37 @@ func TestComposeBuildArgs(t *testing.T) { } } -func TestComposeBuildArgsStructure(t *testing.T) { - t.Run("includes project name, build, and override file", func(t *testing.T) { - args := composeBuildArgs(&composeBuildArgsParams{ - projectName: "ws", - serviceName: "app", - overrideComposeFilePath: "/tmp/override.yml", - }) - if !slices.Contains(args, composeProjectNameFlag) || !slices.Contains(args, "ws") { - t.Errorf("expected project name flag in %v", args) - } - if !slices.Contains(args, "build") { - t.Errorf("expected build verb in %v", args) - } - if !slices.Contains(args, "/tmp/override.yml") { - t.Errorf("expected override file in %v", args) - } +func TestComposeBuildArgsIncludesCoreArgs(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + overrideComposeFilePath: "/tmp/override.yml", }) - t.Run("appends run services without duplicating the main service", func(t *testing.T) { - args := composeBuildArgs(&composeBuildArgsParams{ - projectName: "ws", - serviceName: "app", - runServices: []string{"app", "db"}, - }) - appCount := 0 - for _, a := range args { - if a == "app" { - appCount++ - } - } - if appCount != 1 { - t.Errorf("main service should appear once, got %d in %v", appCount, args) - } - if !slices.Contains(args, "db") { - t.Errorf("expected run service db in %v", args) + for _, want := range []string{composeProjectNameFlag, "ws", "build", "/tmp/override.yml"} { + if !slices.Contains(args, want) { + t.Errorf("expected %q in %v", want, args) } + } +} + +func TestComposeBuildArgsRunServicesNoDuplicate(t *testing.T) { + args := composeBuildArgs(&composeBuildArgsParams{ + projectName: "ws", + serviceName: "app", + runServices: []string{"app", "db"}, }) + + appCount := 0 + for _, a := range args { + if a == "app" { + appCount++ + } + } + if appCount != 1 { + t.Errorf("main service should appear once, got %d in %v", appCount, args) + } + if !slices.Contains(args, "db") { + t.Errorf("expected run service db in %v", args) + } } From 315876e709dd8f5167b66723e3837d410f01938b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Mon, 29 Jun 2026 09:13:27 -0500 Subject: [PATCH 13/13] fix(devcontainer): use resolved image name in compose BuildInfo On the compose build path, when extendResult.buildImageName is empty the code falls back to originalImageName for image inspection but still stored the empty buildImageName in BuildInfo.ImageName, dropping the final image name. Store the resolved currentImageName instead. Pre-existing on main (the refactor only relocated this block); low impact since buildImageName is rarely empty, but BuildInfo.ImageName is consumed downstream (e.g. single-container Image, prebuild). --- pkg/devcontainer/build.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/devcontainer/build.go b/pkg/devcontainer/build.go index 83504c2df..6ed36784c 100644 --- a/pkg/devcontainer/build.go +++ b/pkg/devcontainer/build.go @@ -589,7 +589,7 @@ func (r *runner) composeBuildInfo( return &config.BuildInfo{ ImageDetails: imageDetails, ImageMetadata: extendResult.imageMetadata, - ImageName: extendResult.buildImageName, + ImageName: currentImageName, PrebuildHash: imageTag, RegistryCache: options.RegistryCache, Tags: options.Tag,