From 909f6d12fcd355b3f790163927453443a8bad601 Mon Sep 17 00:00:00 2001 From: Peter Siegel <33677897+yeetypete@users.noreply.github.com> Date: Mon, 6 Apr 2026 20:46:54 +0200 Subject: [PATCH] dev_container: Make volumes key and source key in volume mounts optional (#53137) Fixes some issues concerning volume mounts in the `dev_container` integration: 1. Docker Compose services that don't define a volumes key cause deserialization to fail because the field was required. This field is not strictly necessary, i.e. for other services in a docker compose devcontainer configuration which the editor is not attached to. 1. Volume mounts where source or target is absent (e.g. `tmpfs` mounts that only need a target) also fail to parse because both fields were required. This makes the source key optional, matching the Docker Compose spec. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed devcontainer initialization erroneously requiring each service to have a volumes key. - Fixed devcontainer initialization erroneously requiring source keys for all volume mounts. --------- Co-authored-by: KyleBarton --- crates/dev_container/src/devcontainer_json.rs | 67 +++++++++++-------- .../src/devcontainer_manifest.rs | 13 ++-- crates/dev_container/src/docker.rs | 50 +++++++++++++- 3 files changed, 93 insertions(+), 37 deletions(-) diff --git a/crates/dev_container/src/devcontainer_json.rs b/crates/dev_container/src/devcontainer_json.rs index de970674a4d4ae7b9b583b924addd433d8a03073..84e40edff18c9bb1e15071841afb2648e3fd2cc4 100644 --- a/crates/dev_container/src/devcontainer_json.rs +++ b/crates/dev_container/src/devcontainer_json.rs @@ -60,7 +60,8 @@ pub(crate) enum ShutdownAction { #[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] #[serde(rename_all = "camelCase")] pub(crate) struct MountDefinition { - pub(crate) source: String, + #[serde(default)] + pub(crate) source: Option, pub(crate) target: String, #[serde(rename = "type")] pub(crate) mount_type: Option, @@ -68,23 +69,23 @@ pub(crate) struct MountDefinition { impl Display for MountDefinition { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "type={},source={},target={},consistency=cached", - self.mount_type.clone().unwrap_or_else(|| { - if self.source.starts_with('/') - || self.source.starts_with("\\\\") - || self.source.get(1..3) == Some(":\\") - || self.source.get(1..3) == Some(":/") + let mount_type = self.mount_type.clone().unwrap_or_else(|| { + if let Some(source) = &self.source { + if source.starts_with('/') + || source.starts_with("\\\\") + || source.get(1..3) == Some(":\\") + || source.get(1..3) == Some(":/") { - "bind".to_string() - } else { - "volume".to_string() + return "bind".to_string(); } - }), - self.source, - self.target - ) + } + "volume".to_string() + }); + write!(f, "type={}", mount_type)?; + if let Some(source) = &self.source { + write!(f, ",source={}", source)?; + } + write!(f, ",target={},consistency=cached", self.target) } } @@ -447,8 +448,6 @@ where } } - let source = source - .ok_or_else(|| D::Error::custom(format!("mount string missing 'source': {}", s)))?; let target = target .ok_or_else(|| D::Error::custom(format!("mount string missing 'target': {}", s)))?; @@ -502,9 +501,6 @@ where } } - let source = source.ok_or_else(|| { - D::Error::custom(format!("mount string missing 'source': {}", s)) - })?; let target = target.ok_or_else(|| { D::Error::custom(format!("mount string missing 'target': {}", s)) })?; @@ -873,7 +869,7 @@ mod test { ])), container_user: Some("myUser".to_string()), mounts: Some(vec![MountDefinition { - source: "/localfolder/app".to_string(), + source: Some("/localfolder/app".to_string()), target: "/workspaces/app".to_string(), mount_type: Some("volume".to_string()), }]), @@ -882,7 +878,7 @@ mod test { override_command: Some(true), workspace_folder: Some("/workspaces".to_string()), workspace_mount: Some(MountDefinition { - source: "/app".to_string(), + source: Some("/app".to_string()), target: "/workspaces/app".to_string(), mount_type: Some("bind".to_string()) }), @@ -1316,12 +1312,12 @@ mod test { container_user: Some("myUser".to_string()), mounts: Some(vec![ MountDefinition { - source: "/localfolder/app".to_string(), + source: Some("/localfolder/app".to_string()), target: "/workspaces/app".to_string(), mount_type: Some("volume".to_string()), }, MountDefinition { - source: "dev-containers-cli-bashhistory".to_string(), + source: Some("dev-containers-cli-bashhistory".to_string()), target: "/home/node/commandhistory".to_string(), mount_type: None, } @@ -1331,7 +1327,7 @@ mod test { override_command: Some(true), workspace_folder: Some("/workspaces".to_string()), workspace_mount: Some(MountDefinition { - source: "/folder".to_string(), + source: Some("/folder".to_string()), target: "/workspace".to_string(), mount_type: Some("bind".to_string()) }), @@ -1356,7 +1352,7 @@ mod test { #[test] fn mount_definition_should_use_bind_type_for_unix_absolute_paths() { let mount = MountDefinition { - source: "/home/user/project".to_string(), + source: Some("/home/user/project".to_string()), target: "/workspaces/project".to_string(), mount_type: None, }; @@ -1372,7 +1368,7 @@ mod test { #[test] fn mount_definition_should_use_bind_type_for_windows_unc_paths() { let mount = MountDefinition { - source: "\\\\server\\share\\project".to_string(), + source: Some("\\\\server\\share\\project".to_string()), target: "/workspaces/project".to_string(), mount_type: None, }; @@ -1388,7 +1384,7 @@ mod test { #[test] fn mount_definition_should_use_bind_type_for_windows_absolute_paths() { let mount = MountDefinition { - source: "C:\\Users\\mrg\\cli".to_string(), + source: Some("C:\\Users\\mrg\\cli".to_string()), target: "/workspaces/cli".to_string(), mount_type: None, }; @@ -1400,4 +1396,17 @@ mod test { "Expected mount type 'bind' for Windows absolute path, but got: {rendered}" ); } + + #[test] + fn mount_definition_should_omit_source_when_none() { + let mount = MountDefinition { + source: None, + target: "/tmp".to_string(), + mount_type: Some("tmpfs".to_string()), + }; + + let rendered = mount.to_string(); + + assert_eq!(rendered, "type=tmpfs,target=/tmp,consistency=cached"); + } } diff --git a/crates/dev_container/src/devcontainer_manifest.rs b/crates/dev_container/src/devcontainer_manifest.rs index d28014bffff146ece8cc69f63753ecf5f82a33ea..e3a09ae548b68bb4d589d8a214ca1ba5daa9cfa4 100644 --- a/crates/dev_container/src/devcontainer_manifest.rs +++ b/crates/dev_container/src/devcontainer_manifest.rs @@ -1092,11 +1092,12 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true .filter_map(|mount| { if let Some(mount_type) = &mount.mount_type && mount_type.to_lowercase() == "volume" + && let Some(source) = &mount.source { Some(( - mount.source.clone(), + source.clone(), DockerComposeVolume { - name: mount.source.clone(), + name: source.clone(), }, )) } else { @@ -1744,7 +1745,7 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true }; Ok(MountDefinition { - source: self.local_workspace_folder(), + source: Some(self.local_workspace_folder()), target: format!("/workspaces/{}", project_directory_name.display()), mount_type: None, }) @@ -3576,7 +3577,7 @@ ENV DOCKER_BUILDKIT=1 ])), volumes: vec![ MountDefinition { - source: "dind-var-lib-docker-42dad4b4ca7b8ced".to_string(), + source: Some("dind-var-lib-docker-42dad4b4ca7b8ced".to_string()), target: "/var/lib/docker".to_string(), mount_type: Some("volume".to_string()) } @@ -4644,7 +4645,7 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true additional_contexts: None, }), volumes: vec![MountDefinition { - source: "../..".to_string(), + source: Some("../..".to_string()), target: "/workspaces".to_string(), mount_type: Some("bind".to_string()), }], @@ -4657,7 +4658,7 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true DockerComposeService { image: Some("postgres:14.1".to_string()), volumes: vec![MountDefinition { - source: "postgres-data".to_string(), + source: Some("postgres-data".to_string()), target: "/var/lib/postgresql/data".to_string(), mount_type: Some("volume".to_string()), }], diff --git a/crates/dev_container/src/docker.rs b/crates/dev_container/src/docker.rs index 88600e2b2a5221165b6ca80e36c0ebcfdf35013a..b913aea5fd068fdc75337284f05d99a2266dba05 100644 --- a/crates/dev_container/src/docker.rs +++ b/crates/dev_container/src/docker.rs @@ -141,6 +141,7 @@ pub(crate) struct DockerComposeService { pub(crate) build: Option, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) privileged: Option, + #[serde(default, skip_serializing_if = "Vec::is_empty")] pub(crate) volumes: Vec, #[serde(skip_serializing_if = "Option::is_none")] pub(crate) env_file: Option>, @@ -1042,7 +1043,7 @@ mod test { ), volumes: vec![MountDefinition { mount_type: Some("bind".to_string()), - source: "/path/to".to_string(), + source: Some("/path/to".to_string()), target: "/workspaces".to_string(), }], network_mode: Some("service:db".to_string()), @@ -1072,7 +1073,7 @@ mod test { image: Some("postgres:14.1".to_string()), volumes: vec![MountDefinition { mount_type: Some("volume".to_string()), - source: "postgres-data".to_string(), + source: Some("postgres-data".to_string()), target: "/var/lib/postgresql/data".to_string(), }], ..Default::default() @@ -1164,6 +1165,51 @@ mod test { assert!(config.volumes.is_empty()); } + #[test] + fn should_deserialize_compose_with_missing_volumes_field() { + let given_config = r#" + { + "name": "devcontainer", + "services": { + "sidecar": { + "image": "ubuntu:24.04" + } + } + } + "#; + + let config: DockerComposeConfig = serde_json_lenient::from_str(given_config).unwrap(); + let service = config.services.get("sidecar").unwrap(); + assert!(service.volumes.is_empty()); + } + + #[test] + fn should_deserialize_compose_volume_without_source() { + let given_config = r#" + { + "name": "devcontainer", + "services": { + "app": { + "image": "ubuntu:24.04", + "volumes": [ + { + "type": "tmpfs", + "target": "/tmp" + } + ] + } + } + } + "#; + + let config: DockerComposeConfig = serde_json_lenient::from_str(given_config).unwrap(); + let service = config.services.get("app").unwrap(); + assert_eq!(service.volumes.len(), 1); + assert_eq!(service.volumes[0].source, None); + assert_eq!(service.volumes[0].target, "/tmp"); + assert_eq!(service.volumes[0].mount_type, Some("tmpfs".to_string())); + } + #[test] fn should_deserialize_inspect_without_labels() { let given_config = r#"