Skip to content

Refactor compose mount handling: unify mount definition and disable implicit workspace mounts #94

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jooh merged 2 commits into from
Jun 8, 2026
Merged

Refactor compose mount handling: unify mount definition and disable implicit workspace mounts #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ea8e37
Remove dead compose volume rendering code
jooh Jun 8, 2026
afc357e
Mount compose smoke workspace explicitly
jooh Jun 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions cmd/devcontainer/src/runtime/compose/override_file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@ use super::super::metadata::serialized_container_metadata;
use super::super::paths::unique_temp_path;
use super::service::{self, ServiceDefinition};
use super::ComposeSpec;
use override_mounts::{
compose_additional_volumes, compose_environment, compose_named_volumes,
compose_workspace_volume,
};
use override_mounts::{compose_additional_volumes, compose_environment, compose_named_volumes};
use override_yaml::{
escape_compose_label, escape_compose_scalar, render_compose_string_sequence,
render_compose_volume_entry, render_named_volume_entry,
Expand Down Expand Up @@ -96,11 +93,7 @@ pub(super) fn compose_metadata_override_file(
if let Some(command) = compose_wrapper_command(resolved, service_definition.as_ref())? {
content.push_str(&format!(" command: {command}\n"));
}
let mut volumes = Vec::new();
if let Some(volume) = compose_workspace_volume(resolved, args, remote_workspace_folder) {
volumes.push(volume);
}
volumes.extend(compose_additional_volumes(resolved, args)?);
let volumes = compose_additional_volumes(resolved, args)?;
let named_volumes = compose_named_volumes(&volumes);
if !volumes.is_empty() {
content.push_str("\n volumes:\n");
Expand Down
141 changes: 18 additions & 123 deletions cmd/devcontainer/src/runtime/compose/override_mounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,10 @@

use serde_json::{Map, Number, Value};

use crate::runtime::context::{
additional_mounts_for_workspace_target, workspace_mount_for_args, ResolvedConfig,
};
use crate::runtime::context::ResolvedConfig;
use crate::runtime::mounts::cli_mount_values;
use crate::runtime::mounts::split_mount_options;

pub(super) enum ComposeVolumeEntry {
Short(String),
Long(ComposeMountDefinition),
}

pub(super) struct ComposeNamedVolume {
pub(super) name: String,
pub(super) external: bool,
Expand All @@ -22,37 +15,11 @@ pub(super) struct ComposeMountDefinition {
pub(super) fields: Map<String, Value>,
}

pub(super) fn compose_workspace_volume(
resolved: &ResolvedConfig,
args: &[String],
remote_workspace_folder: &str,
) -> Option<ComposeVolumeEntry> {
let mount = workspace_mount_for_args(resolved, remote_workspace_folder, args);
let definition = compose_mount_definition_from_str(&mount)?;
if definition.mount_type().unwrap_or("bind") != "bind" {
return None;
}
definition
.short_syntax()
.map(ComposeVolumeEntry::Short)
.or(Some(ComposeVolumeEntry::Long(definition)))
}

pub(super) fn compose_additional_volumes(
resolved: &ResolvedConfig,
args: &[String],
) -> Result<Vec<ComposeVolumeEntry>, String> {
) -> Result<Vec<ComposeMountDefinition>, String> {
let mut volumes = Vec::new();
if resolved.configuration.get("workspaceMount").is_none() {
let remote_workspace_folder =
crate::runtime::context::remote_workspace_folder_for_args(resolved, args);
volumes.extend(
additional_mounts_for_workspace_target(resolved, &remote_workspace_folder, args)
.iter()
.filter_map(|mount| compose_mount_definition_from_str(mount))
.map(ComposeVolumeEntry::Long),
);
}
volumes.extend(
resolved
.configuration
Expand All @@ -69,18 +36,14 @@ pub(super) fn compose_additional_volumes(
volumes.extend(
cli_mount_values(args)?
.iter()
.filter_map(|mount| compose_mount_definition_from_str(mount))
.map(ComposeVolumeEntry::Long),
.filter_map(|mount| compose_mount_definition_from_str(mount)),
);
Ok(volumes)
}

pub(super) fn compose_named_volumes(volumes: &[ComposeVolumeEntry]) -> Vec<ComposeNamedVolume> {
pub(super) fn compose_named_volumes(volumes: &[ComposeMountDefinition]) -> Vec<ComposeNamedVolume> {
let mut named_volumes: Vec<ComposeNamedVolume> = Vec::new();
for volume in volumes {
let ComposeVolumeEntry::Long(definition) = volume else {
continue;
};
for definition in volumes {
if definition.mount_type().unwrap_or("bind") != "volume" {
continue;
}
Expand Down Expand Up @@ -114,11 +77,9 @@ pub(super) fn compose_named_volumes(volumes: &[ComposeVolumeEntry]) -> Vec<Compo
named_volumes
}

fn compose_mount_definition(value: &Value) -> Option<ComposeVolumeEntry> {
fn compose_mount_definition(value: &Value) -> Option<ComposeMountDefinition> {
match value {
Value::String(text) => {
compose_mount_definition_from_str(text).map(ComposeVolumeEntry::Long)
}
Value::String(text) => compose_mount_definition_from_str(text),
Value::Object(entries) => {
let mut fields = Map::new();
fields.insert(
Expand Down Expand Up @@ -177,7 +138,7 @@ fn compose_mount_definition(value: &Value) -> Option<ComposeVolumeEntry> {
}
merge_mount_value(&mut fields, key, value.clone());
}
Some(ComposeVolumeEntry::Long(ComposeMountDefinition { fields }))
Some(ComposeMountDefinition { fields })
}
_ => None,
}
Expand Down Expand Up @@ -240,31 +201,6 @@ impl ComposeMountDefinition {
pub(super) fn mount_type(&self) -> Option<&str> {
self.fields.get("type").and_then(Value::as_str)
}

pub(super) fn short_syntax(&self) -> Option<String> {
if self.mount_type().unwrap_or("bind") != "bind" {
return None;
}
if self
.fields
.keys()
.any(|key| !matches!(key.as_str(), "type" | "source" | "target" | "read_only"))
{
return None;
}
let source = self.fields.get("source").and_then(Value::as_str)?;
let target = self.fields.get("target").and_then(Value::as_str)?;
let mut volume = format!("{source}:{target}");
if self
.fields
.get("read_only")
.and_then(Value::as_bool)
.unwrap_or(false)
{
volume.push_str(":ro");
}
Some(volume)
}
}

fn mount_option_key_path(key: &str) -> Vec<&str> {
Expand Down Expand Up @@ -356,19 +292,11 @@ mod tests {
use super::{
compose_mount_definition, compose_mount_definition_from_str, compose_named_volumes,
insert_nested_mount_value, merge_mount_scalar_or_object, parse_mount_option_scalar,
ComposeMountDefinition, ComposeVolumeEntry,
};

fn long_definition(entry: Option<ComposeVolumeEntry>) -> Option<Map<String, Value>> {
match entry? {
ComposeVolumeEntry::Long(definition) => Some(definition.fields),
ComposeVolumeEntry::Short(_) => None,
}
}

#[test]
fn compose_mount_definition_accepts_object_aliases_and_read_only() {
let fields = long_definition(compose_mount_definition(&json!({
let fields = compose_mount_definition(&json!({
"type": "volume",
"src": "cache",
"dst": "/cache",
Expand All @@ -380,8 +308,9 @@ mod tests {
}
},
"consistency": "cached"
})))
.expect("expected long compose mount definition");
}))
.expect("expected compose mount definition")
.fields;

assert_eq!(fields.get("type"), Some(&json!("volume")));
assert_eq!(fields.get("source"), Some(&json!("cache")));
Expand All @@ -404,20 +333,13 @@ mod tests {
}))
.is_none());

let definition = long_definition(compose_mount_definition(&json!({
let definition = compose_mount_definition(&json!({
"type": "volume",
"target": "/cache"
})))
.expect("expected long compose mount definition");
assert_eq!(
ComposeMountDefinition { fields: definition }.short_syntax(),
None
);
assert!(long_definition(None).is_none());
assert!(long_definition(Some(ComposeVolumeEntry::Short(
"/host:/workspace".to_string()
)))
.is_none());
}))
.expect("expected compose mount definition")
.fields;
assert_eq!(definition.get("source"), None);
}

#[test]
Expand Down Expand Up @@ -446,12 +368,6 @@ mod tests {
Some(u64::MAX)
);
assert_eq!(definition.fields.get("ratio"), Some(&json!(1.5)));
assert_eq!(definition.short_syntax(), None);

let readonly = compose_mount_definition_from_str("source=/host,target=/work,readonly")
.expect("readonly bind mount should parse");
assert_eq!(readonly.short_syntax(), Some("/host:/work:ro".to_string()));

let with_false_values = compose_mount_definition_from_str(
"type=volume,src=cache,destination=/cache,external=false,enabled=false",
)
Expand All @@ -461,21 +377,6 @@ mod tests {
Some(&json!({ "external": false }))
);
assert_eq!(with_false_values.fields.get("enabled"), Some(&json!(false)));

let mut no_source = Map::new();
no_source.insert("type".to_string(), json!("bind"));
no_source.insert("target".to_string(), json!("/work"));
assert_eq!(
ComposeMountDefinition { fields: no_source }.short_syntax(),
None
);
let mut no_target = Map::new();
no_target.insert("type".to_string(), json!("bind"));
no_target.insert("source".to_string(), json!("/host"));
assert_eq!(
ComposeMountDefinition { fields: no_target }.short_syntax(),
None
);
}

#[test]
Expand All @@ -491,13 +392,7 @@ mod tests {
let bind = compose_mount_definition_from_str("source=/host,target=/work")
.expect("bind mount should parse");

let named = compose_named_volumes(&[
ComposeVolumeEntry::Short("/host:/work".to_string()),
ComposeVolumeEntry::Long(local),
ComposeVolumeEntry::Long(external),
ComposeVolumeEntry::Long(anonymous),
ComposeVolumeEntry::Long(bind),
]);
let named = compose_named_volumes(&[local, external, anonymous, bind]);

assert_eq!(named.len(), 1);
assert_eq!(named[0].name, "cache");
Expand Down
33 changes: 7 additions & 26 deletions cmd/devcontainer/src/runtime/compose/override_yaml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use serde_json::{Map, Value};

use super::override_mounts::{ComposeNamedVolume, ComposeVolumeEntry};
use super::override_mounts::{ComposeMountDefinition, ComposeNamedVolume};

pub(super) fn escape_compose_label(label: &str) -> String {
label.replace('\'', "''").replace('$', "$$")
Expand All @@ -12,13 +12,8 @@ pub(super) fn escape_compose_scalar(value: &str) -> String {
value.replace('\'', "''").replace('$', "$$")
}

pub(super) fn render_compose_volume_entry(entry: &ComposeVolumeEntry) -> String {
match entry {
ComposeVolumeEntry::Short(volume) => {
format!(" - '{}'\n", escape_compose_scalar(volume))
}
ComposeVolumeEntry::Long(definition) => render_yaml_mapping_list_entry(&definition.fields),
}
pub(super) fn render_compose_volume_entry(definition: &ComposeMountDefinition) -> String {
render_yaml_mapping_list_entry(&definition.fields)
}

pub(super) fn render_compose_string_sequence(values: &[String]) -> Result<String, String> {
Expand Down Expand Up @@ -116,9 +111,7 @@ fn render_yaml_sequence_item(value: &Value, indent: usize) -> String {
mod tests {
use serde_json::{json, Map, Value};

use super::super::override_mounts::{
ComposeMountDefinition, ComposeNamedVolume, ComposeVolumeEntry,
};
use super::super::override_mounts::{ComposeMountDefinition, ComposeNamedVolume};
use super::{
escape_compose_label, escape_compose_scalar, render_compose_string_sequence,
render_compose_volume_entry, render_named_volume_entry, render_yaml_key_value,
Expand All @@ -140,14 +133,7 @@ mod tests {
}

#[test]
fn render_volume_entries_support_short_long_and_named_shapes() {
assert_eq!(
render_compose_volume_entry(&ComposeVolumeEntry::Short(
"/tmp/src:/tmp/dst:$cached".to_string()
)),
" - '/tmp/src:/tmp/dst:$$cached'\n"
);

fn render_volume_entries_support_long_and_named_shapes() {
let mut fields = Map::new();
fields.insert("type".to_string(), Value::String("volume".to_string()));
fields.insert("source".to_string(), Value::String("cache".to_string()));
Expand All @@ -166,10 +152,7 @@ mod tests {
}),
);

let rendered =
render_compose_volume_entry(&ComposeVolumeEntry::Long(ComposeMountDefinition {
fields,
}));
let rendered = render_compose_volume_entry(&ComposeMountDefinition { fields });

assert!(rendered.contains("- optional: null"), "{rendered}");
assert!(rendered.contains("type: 'volume'"), "{rendered}");
Expand All @@ -195,9 +178,7 @@ mod tests {
" scratch:\n"
);
assert_eq!(
render_compose_volume_entry(&ComposeVolumeEntry::Long(ComposeMountDefinition {
fields: Map::new(),
})),
render_compose_volume_entry(&ComposeMountDefinition { fields: Map::new() }),
""
);
}
Expand Down
Loading
Loading