From 358a796a94f62c1acdc13dfd88450d9e20e56364 Mon Sep 17 00:00:00 2001 From: KyleBarton Date: Tue, 7 Apr 2026 10:58:35 -0700 Subject: [PATCH] Appropriately deserialize app_port in devcontainer.json (#53322) 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 Closes #53263 Properly deserializes the app_port field, even if it is an array of strings. Also, removes app_port from consideration when building a docker-compose-based dev container, since the spec specifies that it only applies to non-docker-compose dev containers. Release Notes: - Fixed app_port deserialization in dev container --- crates/dev_container/src/devcontainer_json.rs | 65 ++++++++++++++++--- .../src/devcontainer_manifest.rs | 47 +++----------- 2 files changed, 66 insertions(+), 46 deletions(-) diff --git a/crates/dev_container/src/devcontainer_json.rs b/crates/dev_container/src/devcontainer_json.rs index 84e40edff18c9bb1e15071841afb2648e3fd2cc4..34ee99ed3834d76fbc24afc68aa663df037fa8da 100644 --- a/crates/dev_container/src/devcontainer_json.rs +++ b/crates/dev_container/src/devcontainer_json.rs @@ -217,8 +217,8 @@ pub(crate) struct DevContainer { pub(crate) override_feature_install_order: Option>, pub(crate) customizations: Option, pub(crate) build: Option, - #[serde(default, deserialize_with = "deserialize_string_or_int")] - pub(crate) app_port: Option, + #[serde(default, deserialize_with = "deserialize_app_port")] + pub(crate) app_port: Vec, #[serde(default, deserialize_with = "deserialize_mount_definition")] pub(crate) workspace_mount: Option, pub(crate) workspace_folder: Option, @@ -517,7 +517,7 @@ where Ok(Some(mounts)) } -fn deserialize_string_or_int<'de, D>(deserializer: D) -> Result, D::Error> +fn deserialize_app_port<'de, D>(deserializer: D) -> Result, D::Error> where D: serde::Deserializer<'de>, { @@ -530,9 +530,29 @@ where Int(u32), } - match StringOrInt::deserialize(deserializer)? { - StringOrInt::String(s) => Ok(Some(s)), - StringOrInt::Int(b) => Ok(Some(b.to_string())), + #[derive(Deserialize)] + #[serde(untagged)] + enum AppPort { + Array(Vec), + Single(StringOrInt), + } + + fn normalize_port(value: StringOrInt) -> String { + match value { + StringOrInt::String(s) => { + if s.contains(':') { + s + } else { + format!("{s}:{s}") + } + } + StringOrInt::Int(n) => format!("{n}:{n}"), + } + } + + match AppPort::deserialize(deserializer)? { + AppPort::Single(value) => Ok(vec![normalize_port(value)]), + AppPort::Array(values) => Ok(values.into_iter().map(normalize_port).collect()), } } @@ -862,7 +882,7 @@ mod test { memory: Some("8gb".to_string()), storage: Some("32gb".to_string()), }), - app_port: Some("8081".to_string()), + app_port: vec!["8081:8081".to_string()], container_env: Some(HashMap::from([ ("MYVAR3".to_string(), "myvar3".to_string()), ("MYVAR4".to_string(), "myvar4".to_string()) @@ -1304,7 +1324,7 @@ mod test { memory: Some("8gb".to_string()), storage: Some("32gb".to_string()), }), - app_port: Some("8081".to_string()), + app_port: vec!["8081:8081".to_string()], container_env: Some(HashMap::from([ ("MYVAR3".to_string(), "myvar3".to_string()), ("MYVAR4".to_string(), "myvar4".to_string()) @@ -1349,6 +1369,35 @@ mod test { assert_eq!(devcontainer.build_type(), DevContainerBuildType::Dockerfile); } + #[test] + fn should_deserialize_app_port_array() { + let given_json = r#" + // These are some external comments. serde_lenient should handle them + { + // These are some internal comments + "name": "myDevContainer", + "remoteUser": "root", + "appPort": [ + "8081:8083", + "9001", + ], + "build": { + "dockerfile": "DockerFile", + } + } + "#; + + let result = deserialize_devcontainer_json(given_json); + + assert!(result.is_ok()); + let devcontainer = result.expect("ok"); + + assert_eq!( + devcontainer.app_port, + vec!["8081:8083".to_string(), "9001:9001".to_string()] + ) + } + #[test] fn mount_definition_should_use_bind_type_for_unix_absolute_paths() { let mount = MountDefinition { diff --git a/crates/dev_container/src/devcontainer_manifest.rs b/crates/dev_container/src/devcontainer_manifest.rs index 5ef82fa3eb2a3ac5d13810e0f6102bec4f42295a..29dc0f9fe96d160d9362597fba4e10f86d026604 100644 --- a/crates/dev_container/src/devcontainer_manifest.rs +++ b/crates/dev_container/src/devcontainer_manifest.rs @@ -1229,35 +1229,6 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${{PATH:-\3}}/g' /etc/profile || true } } } - if let Some(port) = &self.dev_container().app_port { - if let Some(network_service_name) = network_mode_service { - if let Some(service) = service_declarations.get_mut(network_service_name) { - service.ports.push(DockerComposeServicePort { - target: port.clone(), - published: port.clone(), - ..Default::default() - }); - } else { - service_declarations.insert( - network_service_name.to_string(), - DockerComposeService { - ports: vec![DockerComposeServicePort { - target: port.clone(), - published: port.clone(), - ..Default::default() - }], - ..Default::default() - }, - ); - } - } else { - main_service.ports.push(DockerComposeServicePort { - target: port.clone(), - published: port.clone(), - ..Default::default() - }); - } - } service_declarations.insert(main_service_name.to_string(), main_service); let new_docker_compose_config = DockerComposeConfig { @@ -1811,9 +1782,10 @@ RUN sed -i -E 's/((^|\s)PATH=)([^\$]*)$/\1\${PATH:-\3}/g' /etc/profile || true } } } - if let Some(app_port) = &self.dev_container().app_port { + for app_port in &self.dev_container().app_port { command.arg("-p"); - command.arg(format!("{app_port}:{app_port}")); + // Should just implement display for an AppPort struct which takes care of this; it might be a custom map like (literally) "8081:8080" + command.arg(app_port); } command.arg("--entrypoint"); @@ -2997,7 +2969,10 @@ mod test { 8082, 8083, ], - "appPort": "8084", + "appPort": [ + 8084, + "8085:8086", + ], "containerEnv": { "VARIABLE_VALUE": "value", @@ -3301,6 +3276,8 @@ chmod +x ./install.sh "8083:8083".to_string(), "-p".to_string(), "8084:8084".to_string(), + "-p".to_string(), + "8085:8086".to_string(), "--entrypoint".to_string(), "/bin/sh".to_string(), "sha256:610e6cfca95280188b021774f8cf69dd6f49bdb6eebc34c5ee2010f4d51cc105".to_string(), @@ -3357,7 +3334,6 @@ chmod +x ./install.sh "db:5432", "db:1234", ], - "appPort": "8084", // Use 'postCreateCommand' to run commands after the container is created. // "postCreateCommand": "rustc --version", @@ -3631,11 +3607,6 @@ ENV DOCKER_BUILDKIT=1 published: "1234".to_string(), ..Default::default() }, - DockerComposeServicePort { - target: "8084".to_string(), - published: "8084".to_string(), - ..Default::default() - }, ], ..Default::default() },