From 6a8eb56faf5e7c8677cf9183967e088455d85527 Mon Sep 17 00:00:00 2001 From: Phil Schalm Date: Thu, 23 Jul 2026 05:11:37 +0000 Subject: [PATCH] dev_container: Build the primary compose service when runServices omits it (#61434) # Objective - Fix a bug that caused a dev container configuration which used both a docker-compose file _and_ `runServices` to incorrectly drop the primary service. ## Solution - Disclosure: Claude wrote the Rust but I understand the implementation - Prior to the change, `run_services` was passed unchanged. However, the dev container CLI adds the primary if it hasn't been specified - [ref this code](https://github.com/devcontainers/cli/blob/f683c29f64a20109b4453e5149807e390ff65133/src/spec-node/dockerCompose.ts#L276-L281) This PR updates Zed to adhere to that behaviour ## Testing - I tested both manually against a minimal reproduction case, as well as a live configuration that was the original reason for creating this PR - I tested this on OS X. While I believe the PR is far enough abstracted from the platform, it's always possible that Windows is doing something weird. I don't expect that, however. - To reproduce either use the following [sample repo](https://github.com/user-attachments/files/30256541/devcontainer-repro.zip) or attempt to build a **clean** docker container for a dev container configuration which utilizes both `runServices` as well as a `docker-compose.yml` file. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [ ] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Dev containers: Fix for docker compose behaviour that caused `runServices` to not implicitly include the primary service if it wasn't explicitly listed --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Chris Biscardi --- .../src/devcontainer_manifest.rs | 176 +++++++++++++++++- 1 file changed, 172 insertions(+), 4 deletions(-) diff --git a/crates/dev_container/src/devcontainer_manifest.rs b/crates/dev_container/src/devcontainer_manifest.rs index 357ecbf1860..d3ee79cf888 100644 --- a/crates/dev_container/src/devcontainer_manifest.rs +++ b/crates/dev_container/src/devcontainer_manifest.rs @@ -1186,11 +1186,13 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true docker_compose_resources.files.push(config_location); let project_name = self.project_name().await?; + let compose_services = + compose_service_list(&main_service_name, dev_container.run_services.as_ref()); self.docker_client .docker_compose_build( &docker_compose_resources.files, &project_name, - dev_container.run_services.as_ref(), + compose_services.as_ref(), ) .await?; ( @@ -1283,11 +1285,13 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true docker_compose_resources.files.push(config_location); let project_name = self.project_name().await?; + let compose_services = + compose_service_list(&main_service_name, dev_container.run_services.as_ref()); self.docker_client .docker_compose_build( &docker_compose_resources.files, &project_name, - dev_container.run_services.as_ref(), + compose_services.as_ref(), ) .await?; @@ -2023,6 +2027,13 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true ) -> Result<(), DevContainerError> { let mut command = Command::new(self.docker_client.docker_cli()); let project_name = self.project_name().await?; + let compose_services = match self.dev_container().run_services.as_ref() { + Some(run_services) => { + let (main_service_name, _) = find_primary_service(&resources, self)?; + compose_service_list(&main_service_name, Some(run_services)) + } + None => None, + }; command.args(&["compose", "--project-name", &project_name]); for docker_compose_file in &resources.files { command.args(&["-f", &docker_compose_file.display().to_string()]); @@ -2031,8 +2042,8 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true if matches!(behavior, ComposeUpBehavior::Resume) { command.arg("--no-recreate"); } - if let Some(run_services) = self.dev_container().run_services.as_ref() { - command.args(run_services); + if let Some(services) = compose_services.as_ref() { + command.args(services); } let output = self @@ -2791,6 +2802,24 @@ fn find_primary_service( } } +/// The compose service list for `build` and `up`. Per the devcontainer spec +/// `runServices` are *additional*, so the primary `service` is always prepended. +/// `None` (no `runServices`) lets compose operate every service. +fn compose_service_list( + main_service_name: &str, + run_services: Option<&Vec>, +) -> Option> { + let run_services = run_services?; + let mut services = Vec::with_capacity(run_services.len() + 1); + services.push(main_service_name.to_string()); + for service in run_services { + if service != main_service_name { + services.push(service.clone()); + } + } + Some(services) +} + /// Resolves a compose service's dockerfile path according to the Docker Compose spec: /// `dockerfile` is relative to the build `context`, and `context` is relative to /// the compose file's directory. @@ -5165,6 +5194,130 @@ ENV DOCKER_BUILDKIT=1 ) } + #[gpui::test] + async fn test_compose_build_includes_primary_service_when_run_services_excludes_it( + cx: &mut TestAppContext, + ) { + cx.executor().allow_parking(); + env_logger::try_init().ok(); + // `app` (the primary `service`) is intentionally left out of `runServices`; + // the spec still implies it, so its features image must still be built. + let given_devcontainer_contents = r#" + { + "features": { + "ghcr.io/devcontainers/features/aws-cli:1": {}, + "ghcr.io/devcontainers/features/docker-in-docker:2": {}, + }, + "name": "Rust and PostgreSQL", + "dockerComposeFile": "docker-compose.yml", + "service": "app", + "runServices": ["db"], + "workspaceFolder": "/workspaces/${localWorkspaceFolderBasename}" + } + "#; + let (test_dependencies, mut devcontainer_manifest) = + init_default_devcontainer_manifest(cx, given_devcontainer_contents) + .await + .unwrap(); + + test_dependencies + .fs + .atomic_write( + PathBuf::from(TEST_PROJECT_PATH).join(".devcontainer/docker-compose.yml"), + r#" +volumes: + postgres-data: + +services: + app: + build: + context: . + dockerfile: Dockerfile + volumes: + - ../..:/workspaces:cached + command: sleep infinity + network_mode: service:db + + db: + image: postgres:14.1 + restart: unless-stopped + volumes: + - postgres-data:/var/lib/postgresql/data + "# + .trim() + .to_string(), + ) + .await + .unwrap(); + + test_dependencies + .fs + .atomic_write( + PathBuf::from(TEST_PROJECT_PATH).join(".devcontainer/Dockerfile"), + r#" +FROM mcr.microsoft.com/devcontainers/rust:2-1-bookworm +RUN apt-get update + "# + .trim() + .to_string(), + ) + .await + .unwrap(); + + devcontainer_manifest.parse_nonremote_vars().unwrap(); + + let _devcontainer_up = devcontainer_manifest.build_and_run().await.unwrap(); + + let recorded = test_dependencies.docker.recorded_compose_build_services(); + let build_services = recorded + .iter() + .find_map(|services| services.clone()) + .expect("docker_compose_build should have been invoked with a service list"); + + assert!( + build_services.iter().any(|s| s == "app"), + "compose build must include the primary service `app` even when \ + runServices omits it; got {build_services:?}" + ); + assert_eq!( + build_services.iter().filter(|s| *s == "app").count(), + 1, + "primary service `app` must appear exactly once (de-duplicated); \ + got {build_services:?}" + ); + + // The implied-primary rule applies to `up -d` too, not just the build. + let docker_commands = test_dependencies + .command_runner + .commands_by_program("docker"); + let compose_up = docker_commands + .iter() + .find(|c| { + c.args.first().map(String::as_str) == Some("compose") + && c.args.iter().any(|a| a == "up") + }) + .expect("docker compose up command recorded"); + let up_index = compose_up + .args + .iter() + .position(|a| a == "up") + .expect("up token present"); + let up_service_args = &compose_up.args[up_index + 2..]; + assert!( + up_service_args.iter().any(|s| s == "app"), + "compose up must start the primary service `app` even when \ + runServices omits it; got {:?}", + compose_up.args + ); + assert_eq!( + up_service_args.iter().filter(|s| *s == "app").count(), + 1, + "primary service `app` must appear exactly once in the up service \ + args (de-duplicated); got {:?}", + compose_up.args + ); + } + #[test] fn derive_project_name_env_wins_over_everything() { // CLI precedence rule 1: `COMPOSE_PROJECT_NAME` env var short-circuits @@ -7141,6 +7294,9 @@ RUN echo $RUBY_VERSION2 /// `MultipleMatchingContainers` with these IDs. Used to exercise the /// duplicate-container error path. duplicate_container_ids: Mutex>>, + /// Records the `services` argument passed to each `docker_compose_build` + /// call so tests can assert which services were built. + compose_build_services: Mutex>>>, } impl FakeDocker { @@ -7150,8 +7306,16 @@ RUN echo $RUBY_VERSION2 has_buildx: true, exec_commands_recorded: Mutex::new(Vec::new()), duplicate_container_ids: Mutex::new(None), + compose_build_services: Mutex::new(Vec::new()), } } + + fn recorded_compose_build_services(&self) -> Vec>> { + self.compose_build_services + .lock() + .expect("should be available") + .clone() + } #[cfg(not(target_os = "windows"))] fn set_podman(&mut self, podman: bool) { self.podman = podman; @@ -7467,6 +7631,10 @@ RUN echo $RUBY_VERSION2 _project_name: &str, _services: Option<&Vec>, ) -> Result<(), DevContainerError> { + self.compose_build_services + .lock() + .expect("should be available") + .push(_services.cloned()); Ok(()) } async fn run_docker_exec(