From 617721a312cbe59f1cbf50f6b1561cb7625da101 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 13:46:56 +0200 Subject: [PATCH 01/12] Reject unknown native CLI options --- build/generate-cli-metadata.js | 30 +++++++++++++++++-- cmd/devcontainer/src/cli.rs | 38 ++++++++++++++---------- cmd/devcontainer/src/cli_metadata.json | 13 ++++++++ cmd/devcontainer/src/lib.rs | 10 +++++-- cmd/devcontainer/tests/cli_smoke/help.rs | 32 ++++++++++++++++++++ 5 files changed, 103 insertions(+), 20 deletions(-) diff --git a/build/generate-cli-metadata.js b/build/generate-cli-metadata.js index 6e04a199c..e54bf305d 100644 --- a/build/generate-cli-metadata.js +++ b/build/generate-cli-metadata.js @@ -33,6 +33,16 @@ const outputPath = path.join( 'cli_metadata.json', ); +const nativeOptionsByCommand = { + up: [ + { + name: 'pull-always', + aliases: [], + description: 'Always pull images before creating the dev container. Native extension. [boolean]', + }, + ], +}; + function readJson(filePath) { return JSON.parse(fs.readFileSync(filePath, 'utf8')); } @@ -161,6 +171,18 @@ function parseDisplayedEntries(lines) { }; } +function nativeOptionsForCommand(commandPath) { + return nativeOptionsByCommand[commandPath] || []; +} + +function nativeOptionLines(options) { + return options.map(option => ({ + text: ` --${option.name.padEnd(32)}${option.description}`, + optionNames: [option.name], + positionalNames: [], + })); +} + function mergeOptions(allOptionNames, displayedOptions) { const displayedByName = new Map(displayedOptions.map(option => [option.name, option])); const merged = allOptionNames.map(name => { @@ -218,14 +240,18 @@ function generateCliMetadata() { runUpstreamHelp(command.path.split(' ')), ); const parsed = parseDisplayedEntries(commandLines); + const nativeOptions = nativeOptionsForCommand(command.path); return { path: command.path, group: command.group, tokenPath: command.path.split(' '), description: command.description, subcommands: groupChildren(matrix, command.path), - lines: parsed.lines, - options: mergeOptions(command.options, parsed.displayedOptions), + lines: [...parsed.lines, ...nativeOptionLines(nativeOptions)], + options: mergeOptions( + [...command.options, ...nativeOptions.map(option => option.name)], + [...parsed.displayedOptions, ...nativeOptions], + ), positionals: parsed.displayedPositionals, unsupportedOptions: unsupportedOptionsForCommand( parityInventory, diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index 2016e50aa..a5b13becd 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -316,17 +316,6 @@ fn unsupported_argument_error_for( command_path: &str, args: &[String], ) -> Option { - let mut unsupported_flags = Vec::new(); - - for option in &command.options { - if command.unsupported_options.contains(&option.name) { - unsupported_flags.push((format!("--{}", option.name), option.name.as_str())); - for alias in &option.aliases { - unsupported_flags.push((format!("-{alias}"), option.name.as_str())); - } - } - } - for arg in args { if arg == "--" { break; @@ -336,16 +325,33 @@ fn unsupported_argument_error_for( break; } + if !arg.starts_with('-') { + continue; + } + let flag = match arg.split_once('=') { Some((name, _)) => name, None => arg.as_str(), }; - for (candidate, _) in &unsupported_flags { - if candidate == flag { - return Some(format!( - "Option {candidate} {UNSUPPORTED_ARGUMENT_MESSAGE}: devcontainer {command_path}" - )); + let short_alias = match flag.strip_prefix('-') { + Some(alias) if !alias.starts_with('-') => Some(alias), + _ => None, + }; + let Some(option) = find_command_option(command, flag, short_alias) else { + if command_path == "up" && flag == "--pull" { + return Some( + "Option --pull is not supported by devcontainer up. Use --pull-always instead." + .to_string(), + ); } + return Some(format!( + "Unknown option: {flag}: devcontainer {command_path}" + )); + }; + if command.unsupported_options.contains(&option.name) { + return Some(format!( + "Option {flag} {UNSUPPORTED_ARGUMENT_MESSAGE}: devcontainer {command_path}" + )); } } diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index 1e5872eb1..977df3e07 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -455,6 +455,13 @@ "include-merged-configuration" ], "positionalNames": [] + }, + { + "text": " --pull-always Always pull images before creating the dev container. Native extension. [boolean]", + "optionNames": [ + "pull-always" + ], + "positionalNames": [] } ], "options": [ @@ -728,6 +735,12 @@ "description": "Workspace mount consistency. [choices: \"consistent\", \"cached\", \"delegated\"] [default: \"cached\"]", "visible": true }, + { + "name": "pull-always", + "aliases": [], + "description": "Always pull images before creating the dev container. Native extension. [boolean]", + "visible": true + }, { "name": "help", "aliases": [], diff --git a/cmd/devcontainer/src/lib.rs b/cmd/devcontainer/src/lib.rs index 08b55251f..e4bed7760 100644 --- a/cmd/devcontainer/src/lib.rs +++ b/cmd/devcontainer/src/lib.rs @@ -33,7 +33,6 @@ pub fn run_from_env() -> ExitCode { run(env::args().skip(1).collect()) } -#[cfg(test)] fn unsupported_argument_exit_code(error: Option) -> Option { match error { Some(error) => { @@ -105,6 +104,13 @@ pub fn run(raw_args: Vec) -> ExitCode { resolved_args, )); + if let Some(exit_code) = unsupported_argument_exit_code(cli::unsupported_argument_error( + resolved_help.path, + &normalized_command_args, + )) { + return exit_code; + } + match commands::dispatch(command, &normalized_command_args) { commands::DispatchResult::Complete(code) => code, commands::DispatchResult::UnsupportedNativePath => { @@ -210,7 +216,7 @@ mod tests { "up".to_string(), "--definitely-unsupported".to_string() ]), - ExitCode::from(1) + ExitCode::from(2) ); assert_eq!( run(vec!["read-configuration".to_string()]), diff --git a/cmd/devcontainer/tests/cli_smoke/help.rs b/cmd/devcontainer/tests/cli_smoke/help.rs index 60244c80a..36f33ed9b 100644 --- a/cmd/devcontainer/tests/cli_smoke/help.rs +++ b/cmd/devcontainer/tests/cli_smoke/help.rs @@ -62,6 +62,38 @@ fn top_level_help_matches_public_cli_surface() { assert!(!stdout.contains("Current state:"), "{stdout}"); } +#[test] +fn up_help_lists_native_pull_always_extension() { + let output = devcontainer_command(None) + .args(["up", "--help"]) + .output() + .expect("up help command should run"); + + assert!(output.status.success(), "{output:?}"); + let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); + assert!(stdout.contains("--pull-always"), "{stdout}"); +} + +#[test] +fn up_rejects_unknown_options_before_dispatch() { + for (option, expected) in [ + ( + "--unrecognized-option", + "Unknown option: --unrecognized-option", + ), + ("--pull=always", "Use --pull-always instead."), + ] { + let output = devcontainer_command(None) + .args(["up", option]) + .output() + .expect("up command should run"); + + assert_eq!(output.status.code(), Some(2), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains(expected), "{stderr}"); + } +} + #[test] fn up_help_lists_upstream_options() { let output = devcontainer_command(None) From 052be4e8ec2973059fa0b85698c22d6158843324 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 13:47:58 +0200 Subject: [PATCH 02/12] Add pull-always support to devcontainer up --- cmd/devcontainer/src/runtime/compose/mod.rs | 3 ++ .../src/runtime/container/engine_run.rs | 11 +++-- cmd/devcontainer/src/runtime/engine.rs | 49 ++++++++++++++++++- .../tests/runtime_container_smoke/basic.rs | 28 +++++++++++ .../runtime_container_smoke/compose_flow.rs | 42 +++++++++++++++- 5 files changed, 126 insertions(+), 7 deletions(-) diff --git a/cmd/devcontainer/src/runtime/compose/mod.rs b/cmd/devcontainer/src/runtime/compose/mod.rs index d57b5a31d..38c9c552d 100644 --- a/cmd/devcontainer/src/runtime/compose/mod.rs +++ b/cmd/devcontainer/src/runtime/compose/mod.rs @@ -164,6 +164,9 @@ pub(crate) fn up_service( }, )?; let mut up_args = vec!["-d".to_string()]; + if common::has_flag(args, "--pull-always") { + up_args.extend(engine::compose_pull_always_args(args)); + } if no_recreate { up_args.push("--no-recreate".to_string()); } diff --git a/cmd/devcontainer/src/runtime/container/engine_run.rs b/cmd/devcontainer/src/runtime/container/engine_run.rs index fb2d725ae..b4a6d5713 100644 --- a/cmd/devcontainer/src/runtime/container/engine_run.rs +++ b/cmd/devcontainer/src/runtime/container/engine_run.rs @@ -59,9 +59,12 @@ fn start_container_with_metadata( let default_labels = common::default_devcontainer_id_labels(&resolved.workspace_folder, &resolved.config_file); let metadata = metadata?; - let mut engine_args = vec![ - "run".to_string(), - "-d".to_string(), + let mut engine_args = vec!["run".to_string(), "-d".to_string()]; + if common::has_flag(args, "--pull-always") { + engine_args.push("--pull".to_string()); + engine_args.push("always".to_string()); + } + engine_args.extend([ "--label".to_string(), default_labels[0].clone(), "--label".to_string(), @@ -70,7 +73,7 @@ fn start_container_with_metadata( format!("devcontainer.metadata={metadata}"), "--mount".to_string(), workspace_mount_for_args(resolved, remote_workspace_folder, args), - ]; + ]); if resolved.configuration.get("workspaceMount").is_none() { for mount in additional_mounts_for_workspace_target(resolved, remote_workspace_folder, args) { diff --git a/cmd/devcontainer/src/runtime/engine.rs b/cmd/devcontainer/src/runtime/engine.rs index 74fe494da..b4aa0c6db 100644 --- a/cmd/devcontainer/src/runtime/engine.rs +++ b/cmd/devcontainer/src/runtime/engine.rs @@ -93,6 +93,28 @@ pub(crate) fn requested_compose_program(args: &[String]) -> Option { ) } +pub(crate) fn compose_pull_always_args(args: &[String]) -> Vec { + if requested_compose_program(args) + .as_deref() + .is_some_and(is_standalone_podman_compose) + { + vec!["--pull-always".to_string()] + } else { + vec!["--pull".to_string(), "always".to_string()] + } +} + +fn is_standalone_podman_compose(program: &str) -> bool { + let filename = program + .rsplit(|character| matches!(character, '/' | '\\')) + .next() + .unwrap_or(program); + Path::new(filename) + .file_stem() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.eq_ignore_ascii_case("podman-compose")) +} + fn default_compose_subcommand_available(args: &[String]) -> bool { let request = common::runtime_process_request( args, @@ -196,8 +218,9 @@ mod tests { use crate::process_runner::{ProcessLogLevel, ProcessRequest, ProcessResult}; use super::{ - compose_request, default_compose_subcommand_available, engine_request, is_build_request, - normalize_process_error, run_compose, run_engine, run_engine_streaming, stderr_or_stdout, + compose_pull_always_args, compose_request, default_compose_subcommand_available, + engine_request, is_build_request, normalize_process_error, run_compose, run_engine, + run_engine_streaming, stderr_or_stdout, }; #[test] @@ -319,6 +342,28 @@ mod tests { assert_eq!(request.program, "/cli/bin/docker"); } + #[test] + fn compose_pull_always_args_match_the_selected_compose_provider() { + assert_eq!( + compose_pull_always_args(&[]), + vec!["--pull".to_string(), "always".to_string()] + ); + assert_eq!( + compose_pull_always_args(&[ + "--docker-compose-path".to_string(), + "/opt/bin/podman-compose".to_string(), + ]), + vec!["--pull-always".to_string()] + ); + assert_eq!( + compose_pull_always_args(&[ + "--docker-compose-path".to_string(), + "C:\\bin\\podman-compose.exe".to_string(), + ]), + vec!["--pull-always".to_string()] + ); + } + #[test] fn compose_request_uses_env_compose_path_before_engine_probe() { let _env = test_env_defaults(&[ diff --git a/cmd/devcontainer/tests/runtime_container_smoke/basic.rs b/cmd/devcontainer/tests/runtime_container_smoke/basic.rs index 173cb3849..d2d7e8dc5 100644 --- a/cmd/devcontainer/tests/runtime_container_smoke/basic.rs +++ b/cmd/devcontainer/tests/runtime_container_smoke/basic.rs @@ -65,6 +65,34 @@ fn up_starts_a_container_and_exec_runs_inside_it() { assert!(exec_log.contains("/bin/sh -lc echo ready")); } +#[test] +fn up_pull_always_forwards_the_engine_pull_policy() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(&workspace).expect("workspace dir"); + write_devcontainer_config(&workspace, "{\n \"image\": \"alpine:3.20\"\n}\n"); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[("FAKE_PODMAN_PS_DISABLE_DEFAULT", "1")], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert!( + invocations.contains("run -d --pull always"), + "{invocations}" + ); +} + #[test] fn up_succeeds_with_env_backed_runtime_defaults() { let harness = RuntimeHarness::new(); diff --git a/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs b/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs index 2b7846645..06ce278ef 100644 --- a/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs +++ b/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs @@ -137,6 +137,42 @@ fn up_starts_compose_services_and_exec_uses_compose_container_lookup() { assert!(exec_log.contains("/bin/sh -lc echo ready")); } +#[test] +fn up_pull_always_forwards_the_compose_pull_policy() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\"\n}\n", + ); + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert!( + invocations.contains(" up -d --pull always"), + "{invocations}" + ); +} + #[test] fn up_generated_override_preserves_compose_version_prefix() { let harness = RuntimeHarness::new(); @@ -398,6 +434,7 @@ fn up_uses_env_backed_engine_and_compose_paths_for_compose_workspaces() { "up", "--workspace-folder", workspace.to_string_lossy().as_ref(), + "--pull-always", ], &[ ("DEVCONTAINER_DOCKER_PATH", fake_podman.as_str()), @@ -410,7 +447,10 @@ fn up_uses_env_backed_engine_and_compose_paths_for_compose_workspaces() { assert_eq!(payload["containerId"], "fake-compose-container-id"); let invocations = harness.read_invocations(); assert!(invocations.contains("compose --project-name workspace_devcontainer -f ")); - assert!(invocations.contains(" up -d")); + assert!( + invocations.contains(" up -d --pull-always"), + "{invocations}" + ); } #[test] From 9cea4e975423950c3415c746082ce2d2de604551 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 13:52:10 +0200 Subject: [PATCH 03/12] Document native CLI extension options --- README.md | 12 +++++ build/generate-cli-metadata.js | 35 ++++++++++++++ cmd/devcontainer/src/cli_metadata.json | 65 ++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) diff --git a/README.md b/README.md index f765189fb..c24b24ebd 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,18 @@ export DEVCONTAINER_DOCKER_COMPOSE_PATH=podman-compose devcontainer up --workspace-folder . ``` +To always refresh a remote image when `up` creates or recreates the container, +use the native `--pull-always` extension: + +```bash +devcontainer up --workspace-folder . --pull-always +``` + +It maps to `--pull always` for Docker, Podman, Docker Compose, and `podman +compose`; standalone `podman-compose` receives its native `--pull-always` +flag. A reused single-container devcontainer does not execute `run`, so combine +this option with `--remove-existing-container` when replacement is required. + To use the same non-standard config path across commands: ```bash diff --git a/build/generate-cli-metadata.js b/build/generate-cli-metadata.js index e54bf305d..678a4d57f 100644 --- a/build/generate-cli-metadata.js +++ b/build/generate-cli-metadata.js @@ -41,6 +41,41 @@ const nativeOptionsByCommand = { description: 'Always pull images before creating the dev container. Native extension. [boolean]', }, ], + build: [ + { + name: 'build-no-cache', + aliases: [], + description: 'Build without using cached layers. Native extension. [boolean]', + }, + ], + 'features test': [ + { + name: 'docker-path', + aliases: [], + description: 'Container engine CLI path. Native extension. [string]', + }, + ], + 'features publish': [ + { + name: 'output-dir', + aliases: [], + description: 'Directory for the generated local OCI layout. Native extension. [string]', + }, + ], + 'templates metadata': [ + { + name: 'workspace-folder', + aliases: [], + description: 'Workspace folder used to resolve local OCI layouts. Native extension. [string]', + }, + ], + 'templates publish': [ + { + name: 'output-dir', + aliases: [], + description: 'Directory for the generated local OCI layout. Native extension. [string]', + }, + ], }; function readJson(filePath) { diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index 977df3e07..de79c7c5d 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -1271,6 +1271,13 @@ "frozen-lockfile" ], "positionalNames": [] + }, + { + "text": " --build-no-cache Build without using cached layers. Native extension. [boolean]", + "optionNames": [ + "build-no-cache" + ], + "positionalNames": [] } ], "options": [ @@ -1418,6 +1425,12 @@ "description": "Workspace folder path. The devcontainer.json will be looked up relative to this path. If not provided, defaults to the current directory. [string]", "visible": true }, + { + "name": "build-no-cache", + "aliases": [], + "description": "Build without using cached layers. Native extension. [boolean]", + "visible": true + }, { "name": "help", "aliases": [], @@ -2751,6 +2764,13 @@ "quiet" ], "positionalNames": [] + }, + { + "text": " --docker-path Container engine CLI path. Native extension. [string]", + "optionNames": [ + "docker-path" + ], + "positionalNames": [] } ], "options": [ @@ -2842,6 +2862,12 @@ "description": "Skip all 'scenario' style tests. Cannot be combined with '--global--scenarios-only'. [boolean] [default: false]", "visible": true }, + { + "name": "docker-path", + "aliases": [], + "description": "Container engine CLI path. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], @@ -3124,9 +3150,22 @@ "log-level" ], "positionalNames": [] + }, + { + "text": " --output-dir Directory for the generated local OCI layout. Native extension. [string]", + "optionNames": [ + "output-dir" + ], + "positionalNames": [] } ], "options": [ + { + "name": "output-dir", + "aliases": [], + "description": "Directory for the generated local OCI layout. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], @@ -3912,9 +3951,22 @@ "log-level" ], "positionalNames": [] + }, + { + "text": " --output-dir Directory for the generated local OCI layout. Native extension. [string]", + "optionNames": [ + "output-dir" + ], + "positionalNames": [] } ], "options": [ + { + "name": "output-dir", + "aliases": [], + "description": "Directory for the generated local OCI layout. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], @@ -4026,6 +4078,13 @@ "log-level" ], "positionalNames": [] + }, + { + "text": " --workspace-folder Workspace folder used to resolve local OCI layouts. Native extension. [string]", + "optionNames": [ + "workspace-folder" + ], + "positionalNames": [] } ], "options": [ @@ -4035,6 +4094,12 @@ "description": "Log level. [choices: \"info\", \"debug\", \"trace\"] [default: \"info\"]", "visible": true }, + { + "name": "workspace-folder", + "aliases": [], + "description": "Workspace folder used to resolve local OCI layouts. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], From 98f57681dc8d8be81338e2c7f5fc4f7ffe5f78f6 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 13:53:09 +0200 Subject: [PATCH 04/12] Satisfy compose provider linting --- cmd/devcontainer/src/runtime/engine.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/cmd/devcontainer/src/runtime/engine.rs b/cmd/devcontainer/src/runtime/engine.rs index b4aa0c6db..0ce143238 100644 --- a/cmd/devcontainer/src/runtime/engine.rs +++ b/cmd/devcontainer/src/runtime/engine.rs @@ -105,10 +105,7 @@ pub(crate) fn compose_pull_always_args(args: &[String]) -> Vec { } fn is_standalone_podman_compose(program: &str) -> bool { - let filename = program - .rsplit(|character| matches!(character, '/' | '\\')) - .next() - .unwrap_or(program); + let filename = program.rsplit(['/', '\\']).next().unwrap_or(program); Path::new(filename) .file_stem() .and_then(|name| name.to_str()) From bed444ca6f8f1c58232978c6f036a3e4c4aa1c09 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 13:56:11 +0200 Subject: [PATCH 05/12] Reject unexpected up arguments --- cmd/devcontainer/src/cli.rs | 31 +++++++++++++++++++++++- cmd/devcontainer/tests/cli_smoke/help.rs | 8 ++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index a5b13becd..c7f062157 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -71,6 +71,18 @@ impl CommandOption { None => false, } } + + fn accepts_explicit_boolean_value(&self, value: Option<&String>) -> bool { + self.description + .as_deref() + .is_none_or(|description| description.contains("[boolean]")) + && value.is_some_and(|value| { + matches!( + value.as_str(), + "false" | "0" | "no" | "off" | "true" | "1" | "yes" | "on" + ) + }) + } } pub struct ResolvedCommandHelp<'a> { @@ -316,7 +328,8 @@ fn unsupported_argument_error_for( command_path: &str, args: &[String], ) -> Option { - for arg in args { + let mut index = 0; + while let Some(arg) = args.get(index) { if arg == "--" { break; } @@ -326,6 +339,12 @@ fn unsupported_argument_error_for( } if !arg.starts_with('-') { + if command_path == "up" { + return Some(format!( + "Unknown argument: {arg}: devcontainer {command_path}" + )); + } + index += 1; continue; } @@ -353,6 +372,16 @@ fn unsupported_argument_error_for( "Option {flag} {UNSUPPORTED_ARGUMENT_MESSAGE}: devcontainer {command_path}" )); } + + let next = args.get(index + 1); + if !arg.contains('=') + && next.is_some_and(|value| value != "--") + && (option.takes_value() || option.accepts_explicit_boolean_value(next)) + { + index += 2; + } else { + index += 1; + } } None diff --git a/cmd/devcontainer/tests/cli_smoke/help.rs b/cmd/devcontainer/tests/cli_smoke/help.rs index 36f33ed9b..883f6247d 100644 --- a/cmd/devcontainer/tests/cli_smoke/help.rs +++ b/cmd/devcontainer/tests/cli_smoke/help.rs @@ -76,15 +76,19 @@ fn up_help_lists_native_pull_always_extension() { #[test] fn up_rejects_unknown_options_before_dispatch() { - for (option, expected) in [ + for (argument, expected) in [ ( "--unrecognized-option", "Unknown option: --unrecognized-option", ), + ( + "unrecognized-argument", + "Unknown argument: unrecognized-argument", + ), ("--pull=always", "Use --pull-always instead."), ] { let output = devcontainer_command(None) - .args(["up", option]) + .args(["up", argument]) .output() .expect("up command should run"); From 553412ac571cde58811db6b8c08f677501d7e7d5 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 14:00:07 +0200 Subject: [PATCH 06/12] Register native command options for validation --- build/generate-cli-metadata.js | 14 ++++++++++++++ cmd/devcontainer/src/cli_metadata.json | 26 ++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/build/generate-cli-metadata.js b/build/generate-cli-metadata.js index 678a4d57f..5492f3bac 100644 --- a/build/generate-cli-metadata.js +++ b/build/generate-cli-metadata.js @@ -48,6 +48,20 @@ const nativeOptionsByCommand = { description: 'Build without using cached layers. Native extension. [boolean]', }, ], + 'set-up': [ + { + name: 'workspace-folder', + aliases: [], + description: 'Workspace folder used to resolve the devcontainer configuration. Native extension. [string]', + }, + ], + exec: [ + { + name: 'secrets-file', + aliases: [], + description: 'Path to a JSON file containing secret environment variables. Native extension. [string]', + }, + ], 'features test': [ { name: 'docker-path', diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index de79c7c5d..2e19f0795 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -951,6 +951,13 @@ "include-merged-configuration" ], "positionalNames": [] + }, + { + "text": " --workspace-folder Workspace folder used to resolve the devcontainer configuration. Native extension. [string]", + "optionNames": [ + "workspace-folder" + ], + "positionalNames": [] } ], "options": [ @@ -1074,6 +1081,12 @@ "description": "Host path to a directory that is intended to be persisted and share state between sessions. [string]", "visible": true }, + { + "name": "workspace-folder", + "aliases": [], + "description": "Workspace folder used to resolve the devcontainer configuration. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], @@ -4441,6 +4454,13 @@ "remote-env" ], "positionalNames": [] + }, + { + "text": " --secrets-file Path to a JSON file containing secret environment variables. Native extension. [string]", + "optionNames": [ + "secrets-file" + ], + "positionalNames": [] } ], "options": [ @@ -4558,6 +4578,12 @@ "description": "Workspace folder path. The devcontainer.json will be looked up relative to this path. If --container-id, --id-label, and --workspace-folder are not provided, this defaults to the current directory. [string]", "visible": true }, + { + "name": "secrets-file", + "aliases": [], + "description": "Path to a JSON file containing secret environment variables. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], From 8b2288e30197877dae83104c4d8a1cccc67144c1 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 15:41:24 +0200 Subject: [PATCH 07/12] Fix template omission clippy lint --- cmd/devcontainer/src/commands/collections/templates.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/devcontainer/src/commands/collections/templates.rs b/cmd/devcontainer/src/commands/collections/templates.rs index 0164a90b2..8eb67e4ad 100644 --- a/cmd/devcontainer/src/commands/collections/templates.rs +++ b/cmd/devcontainer/src/commands/collections/templates.rs @@ -422,10 +422,8 @@ fn template_path_is_omitted(relative_path: &Path, omit_paths: &[String]) -> bool if relative == prefix || relative.starts_with(&format!("{prefix}/")) { return true; } - } else { - if relative == *pattern { - return true; - } + } else if relative == *pattern { + return true; } } false From 7afea95dc0edd91bf10d556cd4172d90b3663b63 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Mon, 17 Aug 2026 15:46:40 +0200 Subject: [PATCH 08/12] Cover unsupported native command dispatch --- cmd/devcontainer/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/devcontainer/src/lib.rs b/cmd/devcontainer/src/lib.rs index e4bed7760..06979e655 100644 --- a/cmd/devcontainer/src/lib.rs +++ b/cmd/devcontainer/src/lib.rs @@ -225,7 +225,7 @@ mod tests { assert_eq!( run(vec![ "read-configuration".to_string(), - "--unsupported".to_string() + "unexpected-positional".to_string() ]), ExitCode::from(2) ); From cdc88d3b0b03f3dab0cc53c3990e2a9950fc3427 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 19 Aug 2026 21:42:06 +0200 Subject: [PATCH 09/12] Allow compose path during set-up validation --- build/generate-cli-metadata.js | 5 +++++ cmd/devcontainer/src/cli.rs | 13 +++++++++++++ cmd/devcontainer/src/cli_metadata.json | 13 +++++++++++++ 3 files changed, 31 insertions(+) diff --git a/build/generate-cli-metadata.js b/build/generate-cli-metadata.js index 5492f3bac..6a7c32a1a 100644 --- a/build/generate-cli-metadata.js +++ b/build/generate-cli-metadata.js @@ -54,6 +54,11 @@ const nativeOptionsByCommand = { aliases: [], description: 'Workspace folder used to resolve the devcontainer configuration. Native extension. [string]', }, + { + name: 'docker-compose-path', + aliases: [], + description: 'Docker Compose CLI path. Native extension. [string]', + }, ], exec: [ { diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index c7f062157..07dc99acb 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -604,6 +604,19 @@ mod tests { assert!(error.is_none()); } + #[test] + fn set_up_accepts_native_compose_path_option() { + let error = unsupported_argument_error( + "set-up", + &[ + "--docker-compose-path".to_string(), + "podman-compose".to_string(), + ], + ); + + assert!(error.is_none(), "{error:?}"); + } + #[test] fn unsupported_argument_error_reports_synthetic_unsupported_options() { let command = CommandHelp { diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index 2e19f0795..b079d9dd2 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -958,6 +958,13 @@ "workspace-folder" ], "positionalNames": [] + }, + { + "text": " --docker-compose-path Docker Compose CLI path. Native extension. [string]", + "optionNames": [ + "docker-compose-path" + ], + "positionalNames": [] } ], "options": [ @@ -1087,6 +1094,12 @@ "description": "Workspace folder used to resolve the devcontainer configuration. Native extension. [string]", "visible": true }, + { + "name": "docker-compose-path", + "aliases": [], + "description": "Docker Compose CLI path. Native extension. [string]", + "visible": true + }, { "name": "help", "aliases": [], From 297c41429d7945e59a63ec97ecae8cc866d4d658 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 19 Aug 2026 22:01:51 +0200 Subject: [PATCH 10/12] Bound Podman package installation retries --- .github/workflows/rust-port-convergence.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/rust-port-convergence.yml b/.github/workflows/rust-port-convergence.yml index 7ce918791..0255fd9a4 100644 --- a/.github/workflows/rust-port-convergence.yml +++ b/.github/workflows/rust-port-convergence.yml @@ -257,8 +257,13 @@ jobs: - name: Install Podman Compose if: matrix.runner == 'podman' run: | - sudo apt-get update - sudo apt-get install -y podman podman-compose + apt_options=( + -o Acquire::Retries=3 + -o Acquire::http::Timeout=20 + -o Acquire::https::Timeout=20 + ) + sudo apt-get "${apt_options[@]}" update + sudo apt-get "${apt_options[@]}" install -y podman podman-compose - name: Runtime version run: | From fbcab8707136e7a98861aa51bdf9481a58222367 Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 19 Aug 2026 22:11:16 +0200 Subject: [PATCH 11/12] Avoid unhealthy Ubuntu package mirror --- .github/workflows/rust-port-convergence.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/rust-port-convergence.yml b/.github/workflows/rust-port-convergence.yml index 0255fd9a4..5ad57dbd7 100644 --- a/.github/workflows/rust-port-convergence.yml +++ b/.github/workflows/rust-port-convergence.yml @@ -257,6 +257,12 @@ jobs: - name: Install Podman Compose if: matrix.runner == 'podman' run: | + # ubuntu-24.04 hosted runners prefer an Azure mirror and only fall + # back after each request times out. Remove that mirror when the + # runner exposes the mirror list so package downloads cannot stall. + if [[ -f /etc/apt/apt-mirrors.txt ]]; then + sudo sed -i '/azure\.archive\.ubuntu\.com/d' /etc/apt/apt-mirrors.txt + fi apt_options=( -o Acquire::Retries=3 -o Acquire::http::Timeout=20 From 49b1fb20707886093f1e4fb616bc80870eca6ffa Mon Sep 17 00:00:00 2001 From: Johan Carlin Date: Wed, 19 Aug 2026 23:22:24 +0200 Subject: [PATCH 12/12] Correct pull and CLI edge-case semantics --- README.md | 16 +- build/generate-cli-metadata.js | 134 ++++++- cmd/devcontainer/src/cli.rs | 321 +++++++++++++++-- cmd/devcontainer/src/cli_metadata.json | 79 +++- .../collections/feature_tests/discovery.rs | 75 ++-- .../commands/collections/feature_tests/mod.rs | 36 +- .../src/commands/collections/mod.rs | 81 +++-- .../src/commands/collections/templates.rs | 3 +- .../collections/tests/feature_tests.rs | 23 +- .../src/commands/collections/tests/mod.rs | 26 +- .../commands/collections/tests/templates.rs | 2 +- cmd/devcontainer/src/commands/common.rs | 11 +- cmd/devcontainer/src/commands/common/args.rs | 67 +++- cmd/devcontainer/src/lib.rs | 10 + cmd/devcontainer/src/runtime/build.rs | 228 +++++++++++- cmd/devcontainer/src/runtime/compose/mod.rs | 323 +++++++++++++++-- cmd/devcontainer/src/runtime/compose/tests.rs | 339 +++++++++++++++++- .../src/runtime/container/engine_run.rs | 28 +- .../src/runtime/container/uid_update/tests.rs | 26 ++ cmd/devcontainer/src/runtime/engine.rs | 115 ++++-- cmd/devcontainer/src/runtime/exec.rs | 66 +++- cmd/devcontainer/src/runtime/mod.rs | 12 +- .../tests/cli_smoke/collections.rs | 93 +++++ cmd/devcontainer/tests/cli_smoke/help.rs | 134 +++++++ cmd/devcontainer/tests/cli_smoke/lockfile.rs | 9 +- .../tests/runtime_container_smoke/basic.rs | 44 ++- .../runtime_container_smoke/compose_flow.rs | 328 ++++++++++++++++- cmd/devcontainer/tests/runtime_exec_smoke.rs | 34 ++ .../support/runtime_harness/fake_engine.rs | 38 ++ docs/upstream/parity-inventory.json | 1 + scripts/standalone/real-engine-smoke.sh | 4 +- 31 files changed, 2406 insertions(+), 300 deletions(-) diff --git a/README.md b/README.md index c24b24ebd..c62a20bc8 100644 --- a/README.md +++ b/README.md @@ -49,10 +49,18 @@ use the native `--pull-always` extension: devcontainer up --workspace-folder . --pull-always ``` -It maps to `--pull always` for Docker, Podman, Docker Compose, and `podman -compose`; standalone `podman-compose` receives its native `--pull-always` -flag. A reused single-container devcontainer does not execute `run`, so combine -this option with `--remove-existing-container` when replacement is required. +For plain image configurations, the runtime explicitly pulls the source image. +For Compose configurations, it resolves, merges, and interpolates the effective +configuration, then determines the active `runServices` set and its dependency +closure. Build-backed services are excluded; for each unique remaining remote +image, it directly invokes the selected engine and checks the exit status of +each call (`pull IMAGE` or `pull --platform PLATFORM IMAGE`). +Dockerfile, Compose, and Feature builds use build-stage `--pull` where needed to +refresh remote bases, without force-pulling locally generated final tags such as +Feature or UID-update images. During `up`, this refresh happens before the final +existing-container lookup and reuse/start/create decision. An existing container +can therefore still be reused after its source is refreshed; combine the option +with `--remove-existing-container` when replacement is required. To use the same non-standard config path across commands: diff --git a/build/generate-cli-metadata.js b/build/generate-cli-metadata.js index 6a7c32a1a..c6323b5e4 100644 --- a/build/generate-cli-metadata.js +++ b/build/generate-cli-metadata.js @@ -97,6 +97,57 @@ const nativeOptionsByCommand = { ], }; +// Native collection handlers retain positional forms that are not declared by +// the pinned upstream yargs command definitions. Keep them in validation +// metadata without changing the upstream-rendered help text. +const nativePositionalsByCommand = { + 'templates apply': [ + { + name: 'target', + description: 'Local template folder to apply. Native extension. [string]', + }, + ], + 'features generate-docs': [ + { + name: 'target', + description: 'Feature collection folder. Native extension. [string]', + }, + ], + 'templates generate-docs': [ + { + name: 'target', + description: 'Template collection folder. Native extension. [string]', + }, + ], +}; + +// Hidden upstream options do not appear in rendered help, so retain the type +// and alias details needed to parse their values without making them visible. +const hiddenOptionsByCommand = { + 'features test': [ + { + name: 'projectFolder', + aliases: [], + description: 'Project folder accepted by the native feature test runner. [string]', + visible: false, + }, + ], + upgrade: [ + { + name: 'feature', + aliases: ['f'], + description: 'Feature identifier to upgrade. [string]', + visible: false, + }, + { + name: 'target-version', + aliases: ['v'], + description: 'Feature version requirement to apply. [string]', + visible: false, + }, + ], +}; + function readJson(filePath) { return JSON.parse(fs.readFileSync(filePath, 'utf8')); } @@ -171,17 +222,22 @@ function parseOptionLine(line) { } function parsePositionalLine(line) { - const columns = splitHelpColumns(line); - if (!columns) { + // Yargs renders long positional descriptions on continuation lines, leaving + // the first line as just " ". Only consider entries at the section's + // two-space indentation so those continuation lines are not mistaken for + // additional positionals. + if (!/^ {2}\S/.test(line)) { return null; } - const name = columns.label.split(/\s+/)[0]; - if (!name || name.startsWith('-')) { + const columns = splitHelpColumns(line); + const label = columns ? columns.label : line.trim(); + const name = label.split(/\s+/)[0]; + if (!name || name.startsWith('-') || name.startsWith('[')) { return null; } return { name, - description: columns.description, + description: columns ? columns.description : '', }; } @@ -225,10 +281,42 @@ function parseDisplayedEntries(lines) { }; } +function mergeUsagePositionals(commandPath, lines, displayedPositionals) { + const usagePrefix = `devcontainer ${commandPath}`; + const usageLine = lines[0] || ''; + if (!usageLine.startsWith(usagePrefix)) { + return displayedPositionals; + } + + const byName = new Map(displayedPositionals.map(positional => [positional.name, positional])); + const merged = []; + for (const match of usageLine.slice(usagePrefix.length).matchAll(/<([^>]+)>|\[([^\]]+)\]/g)) { + const name = (match[1] || match[2]).replace(/\.\.$/, ''); + if (!byName.has(name)) { + byName.set(name, { name, description: '' }); + } + merged.push(byName.get(name)); + } + for (const positional of displayedPositionals) { + if (!merged.some(candidate => candidate.name === positional.name)) { + merged.push(positional); + } + } + return merged; +} + function nativeOptionsForCommand(commandPath) { return nativeOptionsByCommand[commandPath] || []; } +function nativePositionalsForCommand(commandPath) { + return nativePositionalsByCommand[commandPath] || []; +} + +function hiddenOptionsForCommand(commandPath) { + return hiddenOptionsByCommand[commandPath] || []; +} + function nativeOptionLines(options) { return options.map(option => ({ text: ` --${option.name.padEnd(32)}${option.description}`, @@ -239,20 +327,27 @@ function nativeOptionLines(options) { function mergeOptions(allOptionNames, displayedOptions) { const displayedByName = new Map(displayedOptions.map(option => [option.name, option])); - const merged = allOptionNames.map(name => { + const merged = []; + const seenNames = new Set(); + for (const name of allOptionNames) { + if (seenNames.has(name)) { + continue; + } + seenNames.add(name); const displayed = displayedByName.get(name); - return { + merged.push({ name, aliases: displayed ? displayed.aliases : [], description: displayed ? displayed.description : null, - visible: Boolean(displayed), - }; - }); + visible: Boolean(displayed) && displayed.visible !== false, + }); + } for (const displayed of displayedOptions) { - if (!displayedByName.has(displayed.name) || allOptionNames.includes(displayed.name)) { + if (seenNames.has(displayed.name)) { continue; } + seenNames.add(displayed.name); merged.push({ name: displayed.name, aliases: displayed.aliases, @@ -295,6 +390,13 @@ function generateCliMetadata() { ); const parsed = parseDisplayedEntries(commandLines); const nativeOptions = nativeOptionsForCommand(command.path); + const nativePositionals = nativePositionalsForCommand(command.path); + const hiddenOptions = hiddenOptionsForCommand(command.path); + const positionals = mergeUsagePositionals( + command.path, + commandLines, + [...parsed.displayedPositionals, ...nativePositionals], + ); return { path: command.path, group: command.group, @@ -303,10 +405,14 @@ function generateCliMetadata() { subcommands: groupChildren(matrix, command.path), lines: [...parsed.lines, ...nativeOptionLines(nativeOptions)], options: mergeOptions( - [...command.options, ...nativeOptions.map(option => option.name)], - [...parsed.displayedOptions, ...nativeOptions], + [ + ...command.options, + ...nativeOptions.map(option => option.name), + ...hiddenOptions.map(option => option.name), + ], + [...parsed.displayedOptions, ...nativeOptions, ...hiddenOptions], ), - positionals: parsed.displayedPositionals, + positionals, unsupportedOptions: unsupportedOptionsForCommand( parityInventory, command.path, diff --git a/cmd/devcontainer/src/cli.rs b/cmd/devcontainer/src/cli.rs index 07dc99acb..8dccd64b0 100644 --- a/cmd/devcontainer/src/cli.rs +++ b/cmd/devcontainer/src/cli.rs @@ -50,8 +50,10 @@ struct HelpLine { struct CommandHelp { path: String, token_path: Vec, + subcommands: Vec, lines: Vec, options: Vec, + positionals: Vec, unsupported_options: Vec, unsupported_positionals: Vec, } @@ -64,6 +66,12 @@ struct CommandOption { description: Option, } +#[derive(Debug, Deserialize)] +struct CommandPositional { + name: String, + description: String, +} + impl CommandOption { fn takes_value(&self) -> bool { match self.description.as_deref() { @@ -83,6 +91,18 @@ impl CommandOption { ) }) } + + fn takes_multiple_values(&self) -> bool { + self.description + .as_deref() + .is_some_and(|description| description.contains("[array]")) + } +} + +impl CommandPositional { + fn takes_multiple_values(&self) -> bool { + self.description.contains("[array]") + } } pub struct ResolvedCommandHelp<'a> { @@ -247,25 +267,21 @@ pub(crate) fn normalize_option_aliases(command_path: &str, args: &[String]) -> V normalized.extend_from_slice(&args[index..]); break; } - let flag = match arg.split_once('=') { - Some((name, _)) => name, - None => arg.as_str(), + let (flag, inline_value) = match arg.split_once('=') { + Some((name, value)) => (name, Some(value)), + None => (arg.as_str(), None), }; - let short_alias = match arg.strip_prefix('-') { + let short_alias = match flag.strip_prefix('-') { Some(value) if !value.starts_with('-') => Some(value), _ => None, }; let option = find_command_option(command, flag, short_alias); if let Some(option) = option { - if arg.contains('=') { - normalized.push(arg.clone()); - } else if match short_alias { - Some(alias) => option_has_alias(option, alias), - None => false, - } { - normalized.push(format!("--{}", option.name)); - } else { - normalized.push(arg.clone()); + normalized.push(format!("--{}", option.name)); + if let Some(value) = inline_value { + normalized.push(value.to_string()); + index += 1; + continue; } } else { normalized.push(arg.clone()); @@ -278,7 +294,7 @@ pub(crate) fn normalize_option_aliases(command_path: &str, args: &[String]) -> V Some(option) => option.takes_value(), None => false, }; - if option_takes_value && !arg.contains('=') && next_arg_is_value { + if option_takes_value && next_arg_is_value { index += 1; normalized.push(args[index].clone()); } @@ -317,10 +333,69 @@ fn option_has_alias(option: &CommandOption, alias: &str) -> bool { false } +pub(crate) fn command_positionals(command_path: &str, args: &[String]) -> Vec { + let Some(command) = command_help(command_path) else { + return Vec::new(); + }; + let mut positionals = Vec::new(); + let mut options_ended = false; + let mut index = 0; + while let Some(arg) = args.get(index) { + if !options_ended && arg == "--" { + options_ended = true; + index += 1; + continue; + } + if options_ended || !arg.starts_with('-') { + positionals.push(arg.clone()); + index += 1; + continue; + } + + let flag = arg.split_once('=').map_or(arg.as_str(), |(name, _)| name); + let short_alias = match flag.strip_prefix('-') { + Some(alias) if !alias.starts_with('-') => Some(alias), + _ => None, + }; + let Some(option) = find_command_option(command, flag, short_alias) else { + index += 1; + continue; + }; + if arg.contains('=') { + index += 1; + } else if option.takes_multiple_values() { + index += 1; + while args + .get(index) + .is_some_and(|value| value != "--" && !value.starts_with('-')) + { + index += 1; + } + } else if args.get(index + 1).is_some_and(|value| { + value != "--" + && (option.takes_value() || option.accepts_explicit_boolean_value(Some(value))) + }) { + index += 2; + } else { + index += 1; + } + } + positionals +} + pub fn unsupported_argument_error(command_path: &str, args: &[String]) -> Option { let command = command_help(command_path)?; - unsupported_argument_error_for(command, command_path, args) + // Dispatch retains resolved nested command tokens (for example `test` in + // `features test`) in its argument vector. They identify the command and + // are not positionals declared by the leaf command's metadata. + let nested_tokens = &command.token_path[1..]; + let command_args = match args.strip_prefix(nested_tokens) { + Some(command_args) => command_args, + None => args, + }; + + unsupported_argument_error_for(command, command_path, command_args) } fn unsupported_argument_error_for( @@ -329,20 +404,41 @@ fn unsupported_argument_error_for( args: &[String], ) -> Option { let mut index = 0; + let mut positional_index = 0; + let mut options_ended = false; while let Some(arg) = args.get(index) { - if arg == "--" { - break; + if !options_ended && arg == "--" { + if command.path == "exec" { + break; + } + options_ended = true; + index += 1; + continue; } - if command.path == "exec" && !arg.starts_with('-') { + if !options_ended && command.path == "exec" && !arg.starts_with('-') { break; } - if !arg.starts_with('-') { - if command_path == "up" { - return Some(format!( - "Unknown argument: {arg}: devcontainer {command_path}" - )); + if options_ended || !arg.starts_with('-') { + // Group commands still need to pass unknown tokens through to the + // dispatcher so it can report an unsupported subcommand. Leaf + // commands, however, accept only the positionals in their + // generated upstream metadata. + if command.subcommands.is_empty() { + let Some(positional) = command.positionals.get(positional_index) else { + return Some(format!( + "Unknown argument: {arg}: devcontainer {command_path}" + )); + }; + if command.unsupported_positionals.contains(&positional.name) { + return Some(format!( + "Argument {arg} {UNSUPPORTED_ARGUMENT_MESSAGE}: devcontainer {command_path}" + )); + } + if !positional.takes_multiple_values() { + positional_index += 1; + } } index += 1; continue; @@ -374,7 +470,15 @@ fn unsupported_argument_error_for( } let next = args.get(index + 1); - if !arg.contains('=') + if !arg.contains('=') && option.takes_multiple_values() { + index += 1; + while args + .get(index) + .is_some_and(|value| value != "--" && !value.starts_with('-')) + { + index += 1; + } + } else if !arg.contains('=') && next.is_some_and(|value| value != "--") && (option.takes_value() || option.accepts_explicit_boolean_value(next)) { @@ -390,10 +494,10 @@ fn unsupported_argument_error_for( #[cfg(test)] mod tests { use super::{ - command_help, command_help_text, is_command_help_request, is_command_version_request, - normalize_option_aliases, rendered_cli_log, rendered_lines, resolve_command_help, - unsupported_argument_error, unsupported_argument_error_for, CommandHelp, CommandOption, - HelpLine, + cli_metadata, command_help, command_help_text, command_positionals, + is_command_help_request, is_command_version_request, normalize_option_aliases, + rendered_cli_log, rendered_lines, resolve_command_help, unsupported_argument_error, + unsupported_argument_error_for, CommandHelp, CommandOption, CommandPositional, HelpLine, }; #[test] @@ -457,7 +561,7 @@ mod tests { } #[test] - fn preserves_long_options_with_inline_values() { + fn normalizes_inline_long_and_short_option_values() { let normalized = normalize_option_aliases( "templates apply", &[ @@ -470,11 +574,46 @@ mod tests { assert_eq!( normalized, vec![ - "--workspace-folder=/tmp/workspace".to_string(), - "--template-id=ghcr.io/devcontainers/templates/docker-from-docker:latest" - .to_string(), + "--workspace-folder".to_string(), + "/tmp/workspace".to_string(), + "--template-id".to_string(), + "ghcr.io/devcontainers/templates/docker-from-docker:latest".to_string(), ] ); + + let normalized = normalize_option_aliases( + "upgrade", + &[ + "-f=ghcr.io/example/features/demo".to_string(), + "-v=2".to_string(), + ], + ); + assert_eq!( + normalized, + vec![ + "--feature".to_string(), + "ghcr.io/example/features/demo".to_string(), + "--target-version".to_string(), + "2".to_string(), + ] + ); + } + + #[test] + fn extracts_positionals_across_option_and_separator_forms() { + assert!(command_positionals("unknown", &["target".to_string()]).is_empty()); + assert_eq!( + command_positionals( + "templates apply", + &[ + "--workspace-folder=/workspace".to_string(), + "--unknown".to_string(), + "--".to_string(), + "--target".to_string(), + ], + ), + vec!["--target"] + ); } #[test] @@ -594,6 +733,21 @@ mod tests { assert!(upgrade.unsupported_options.is_empty()); } + #[test] + fn generated_command_option_names_are_unique() { + for command in &cli_metadata().commands { + let mut names = std::collections::HashSet::new(); + for option in &command.options { + assert!( + names.insert(option.name.as_str()), + "duplicate option metadata for {}: {}", + command.path, + option.name + ); + } + } + } + #[test] fn supported_command_options_are_not_reported_as_unsupported() { let error = unsupported_argument_error( @@ -604,6 +758,24 @@ mod tests { assert!(error.is_none()); } + #[test] + fn features_test_accepts_hidden_camel_case_project_folder_alias() { + let error = unsupported_argument_error( + "features test", + &[ + "test".to_string(), + "--projectFolder".to_string(), + "/workspace".to_string(), + ], + ); + + assert!(error.is_none(), "{error:?}"); + assert!( + !command_help_text("features test").contains("--projectFolder"), + "hidden alias should not be duplicated in help" + ); + } + #[test] fn set_up_accepts_native_compose_path_option() { let error = unsupported_argument_error( @@ -622,14 +794,19 @@ mod tests { let command = CommandHelp { path: "sample".to_string(), token_path: vec!["sample".to_string()], + subcommands: Vec::new(), lines: Vec::new(), options: vec![CommandOption { name: "legacy".to_string(), aliases: vec!["l".to_string()], description: Some("Legacy option".to_string()), }], + positionals: vec![CommandPositional { + name: "legacy-target".to_string(), + description: "Legacy target [string]".to_string(), + }], unsupported_options: vec!["legacy".to_string()], - unsupported_positionals: Vec::new(), + unsupported_positionals: vec!["legacy-target".to_string()], }; let error = unsupported_argument_error_for(&command, "sample", &["-l".to_string()]) @@ -647,7 +824,20 @@ mod tests { "sample", &["--".to_string(), "-l".to_string()], ); - assert!(after_separator.is_none()); + assert_eq!( + after_separator.as_deref(), + Some( + "Argument -l is recognized for this command but is not yet implemented in the native Rust CLI: devcontainer sample" + ) + ); + + let positional_error = + unsupported_argument_error_for(&command, "sample", &["target".to_string()]) + .expect("unsupported positional"); + assert_eq!( + positional_error, + "Argument target is recognized for this command but is not yet implemented in the native Rust CLI: devcontainer sample" + ); } #[test] @@ -666,11 +856,74 @@ mod tests { #[test] fn preserves_positional_metadata_for_nested_commands() { + let build = command_help("build").expect("build metadata"); + assert_eq!(build.positionals[0].name, "path"); + let command = command_help("features test").expect("features test metadata"); assert!(command .lines .iter() .any(|line| line.positional_names.contains(&"target".to_string()))); + assert_eq!(command.positionals[0].name, "target"); + + for path in [ + "features package", + "features publish", + "templates publish", + "templates apply", + "features generate-docs", + "templates generate-docs", + ] { + let command = command_help(path).expect("nested command metadata"); + assert_eq!(command.positionals[0].name, "target", "{path}"); + } + } + + #[test] + fn accepts_all_values_of_array_options_before_validating_positionals() { + let error = unsupported_argument_error( + "features test", + &[ + "test".to_string(), + "--features".to_string(), + "one".to_string(), + "two".to_string(), + "--quiet".to_string(), + ], + ); + + assert!(error.is_none(), "{error:?}"); + } + + #[test] + fn accepts_declared_positionals_and_rejects_extras() { + assert!(unsupported_argument_error("build", &["workspace".to_string()]).is_none()); + + let error = + unsupported_argument_error("build", &["workspace".to_string(), "extra".to_string()]); + assert_eq!( + error.as_deref(), + Some("Unknown argument: extra: devcontainer build") + ); + + assert!(unsupported_argument_error( + "features info", + &[ + "info".to_string(), + "manifest".to_string(), + "ghcr.io/example/features/sample:1".to_string(), + ], + ) + .is_none()); + + assert!(unsupported_argument_error( + "features info", + &[ + "manifest".to_string(), + "ghcr.io/example/features/sample:1".to_string(), + ], + ) + .is_none()); } #[test] diff --git a/cmd/devcontainer/src/cli_metadata.json b/cmd/devcontainer/src/cli_metadata.json index b079d9dd2..d5945e9b8 100644 --- a/cmd/devcontainer/src/cli_metadata.json +++ b/cmd/devcontainer/src/cli_metadata.json @@ -1470,7 +1470,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "path", + "description": "" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -2481,8 +2486,10 @@ }, { "name": "feature", - "aliases": [], - "description": null, + "aliases": [ + "f" + ], + "description": "Feature identifier to upgrade. [string]", "visible": false }, { @@ -2493,8 +2500,10 @@ }, { "name": "target-version", - "aliases": [], - "description": null, + "aliases": [ + "v" + ], + "description": "Feature version requirement to apply. [string]", "visible": false }, { @@ -2894,6 +2903,12 @@ "description": "Container engine CLI path. Native extension. [string]", "visible": true }, + { + "name": "projectFolder", + "aliases": [], + "description": "Project folder accepted by the native feature test runner. [string]", + "visible": false + }, { "name": "help", "aliases": [], @@ -2954,7 +2969,9 @@ { "text": " target", "optionNames": [], - "positionalNames": [] + "positionalNames": [ + "target" + ] }, { "text": " Package features at provided [target] (default is cwd), where [target] is either:", @@ -3068,7 +3085,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -3110,7 +3132,9 @@ { "text": " target", "optionNames": [], - "positionalNames": [] + "positionalNames": [ + "target" + ] }, { "text": " Package and publish features at provided [target] (default is cwd), where [target] is either:", @@ -3227,7 +3251,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -3600,7 +3629,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "Feature collection folder. Native extension. [string]" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -3869,7 +3903,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "Local template folder to apply. Native extension. [string]" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -3911,7 +3950,9 @@ { "text": " target", "optionNames": [], - "positionalNames": [] + "positionalNames": [ + "target" + ] }, { "text": " Package and publish templates at provided [target] (default is cwd), where [target] is either:", @@ -4028,7 +4069,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, @@ -4266,7 +4312,12 @@ "visible": true } ], - "positionals": [], + "positionals": [ + { + "name": "target", + "description": "Template collection folder. Native extension. [string]" + } + ], "unsupportedOptions": [], "unsupportedPositionals": [] }, diff --git a/cmd/devcontainer/src/commands/collections/feature_tests/discovery.rs b/cmd/devcontainer/src/commands/collections/feature_tests/discovery.rs index a59d0f124..521bb9c01 100644 --- a/cmd/devcontainer/src/commands/collections/feature_tests/discovery.rs +++ b/cmd/devcontainer/src/commands/collections/feature_tests/discovery.rs @@ -17,12 +17,9 @@ use super::{ use crate::commands::common; pub(super) fn discover_feature_test_cases(args: &[String]) -> Result, String> { - let project_folder = match feature_test_project_folder_arg(args) { - Some(project_folder) => project_folder, - None => return Err("features test requires a project folder".to_string()), - }; + let project_folder = feature_test_project_folder_arg(args); let filter = common::parse_option_value(args, "--filter"); - let feature_filter = feature_filter_arg(args); + let feature_filters = feature_filter_args(args); let skip_scenarios = common::has_flag(args, "--skip-scenarios"); let global_scenarios_only = common::has_flag(args, "--global-scenarios-only"); let skip_autogenerated = common::has_flag(args, "--skip-autogenerated"); @@ -42,7 +39,7 @@ pub(super) fn discover_feature_test_cases(args: &[String]) -> Result Result Result Option { +fn feature_test_project_folder_arg(args: &[String]) -> String { if let Some(project_folder) = common::parse_option_value(args, "--project-folder") { - return Some(project_folder); + return project_folder; } if let Some(project_folder) = common::parse_option_value(args, "--projectFolder") { - return Some(project_folder); - } - for arg in args.iter().rev() { - if !arg.starts_with('-') { - return Some(arg.clone()); - } + return project_folder; } - None + crate::cli::command_positionals("features test", args) + .into_iter() + .next() + .unwrap_or_else(|| ".".to_string()) } -fn feature_filter_arg(args: &[String]) -> Option { - match common::parse_option_value(args, "-f") { - Some(feature_filter) => Some(feature_filter), - None => common::parse_option_value(args, "--features"), +fn feature_filter_args(args: &[String]) -> Vec { + let short = common::parse_array_option_values(args, "-f"); + if short.is_empty() { + common::parse_array_option_values(args, "--features") + } else { + short } } -fn feature_filter_excludes(feature_filter: &Option, feature_name: &str) -> bool { - match feature_filter.as_deref() { - Some(filter) => filter != feature_name, - None => false, - } +fn feature_filter_excludes(feature_filters: &[String], feature_name: &str) -> bool { + !feature_filters.is_empty() && !feature_filters.iter().any(|filter| filter == feature_name) } fn load_named_scenarios(test_dir: &Path) -> Result, String> { @@ -268,7 +262,7 @@ mod tests { use serde_json::Value; use super::{ - discover_feature_test_cases, feature_filter_arg, feature_filter_excludes, + discover_feature_test_cases, feature_filter_args, feature_filter_excludes, feature_test_project_folder_arg, prepare_feature_test_case, prepare_feature_test_case_in_workspace, FeatureTestCase, FeatureTestExecution, FEATURE_TEST_LIBRARY_SCRIPT_NAME, @@ -311,32 +305,33 @@ mod tests { fn feature_test_project_folder_arg_uses_trailing_positionals() { let args = vec!["--quiet".to_string(), "/workspace/project".to_string()]; - assert_eq!( - feature_test_project_folder_arg(&args).as_deref(), - Some("/workspace/project") - ); + assert_eq!(feature_test_project_folder_arg(&args), "/workspace/project"); assert_eq!( feature_test_project_folder_arg(&["--quiet".to_string()]), - None + "." ); } #[test] - fn feature_filter_arg_prefers_short_flag_and_exclusion_matches_names() { + fn feature_filter_args_support_arrays_and_exclusion_matches_names() { let args = vec![ - "--features".to_string(), - "from-long".to_string(), "-f".to_string(), "from-short".to_string(), + "other-short".to_string(), + "--features".to_string(), + "from-long".to_string(), ]; let long_only = vec!["--features".to_string(), "from-long".to_string()]; - assert_eq!(feature_filter_arg(&args).as_deref(), Some("from-short")); - assert_eq!(feature_filter_arg(&long_only).as_deref(), Some("from-long")); - assert_eq!(feature_filter_arg(&[]), None); - assert!(!feature_filter_excludes(&Some("demo".to_string()), "demo")); - assert!(feature_filter_excludes(&Some("other".to_string()), "demo")); - assert!(!feature_filter_excludes(&None, "demo")); + assert_eq!( + feature_filter_args(&args), + vec!["from-short", "other-short"] + ); + assert_eq!(feature_filter_args(&long_only), vec!["from-long"]); + assert!(feature_filter_args(&[]).is_empty()); + assert!(!feature_filter_excludes(&["demo".to_string()], "demo")); + assert!(feature_filter_excludes(&["other".to_string()], "demo")); + assert!(!feature_filter_excludes(&[], "demo")); } #[test] diff --git a/cmd/devcontainer/src/commands/collections/feature_tests/mod.rs b/cmd/devcontainer/src/commands/collections/feature_tests/mod.rs index c38cf9eec..13a83a6d5 100644 --- a/cmd/devcontainer/src/commands/collections/feature_tests/mod.rs +++ b/cmd/devcontainer/src/commands/collections/feature_tests/mod.rs @@ -201,10 +201,7 @@ pub(super) fn execute_feature_tests_with_runtime( } fn parse_feature_test_options(args: &[String]) -> Result { - let project_folder = match feature_test_project_folder_arg(args) { - Some(project_folder) => PathBuf::from(project_folder), - None => return Err("features test requires a project folder".to_string()), - }; + let project_folder = PathBuf::from(feature_test_project_folder_arg(args)); let base_image = common::parse_option_value(args, "--base-image") .unwrap_or(DEFAULT_FEATURE_TEST_BASE_IMAGE.to_string()); let remote_user = common::parse_option_value(args, "--remote-user"); @@ -230,19 +227,17 @@ fn all_feature_tests_passed(results: &[FeatureTestResult]) -> bool { true } -fn feature_test_project_folder_arg(args: &[String]) -> Option { +fn feature_test_project_folder_arg(args: &[String]) -> String { if let Some(project_folder) = common::parse_option_value(args, "--project-folder") { - return Some(project_folder); + return project_folder; } if let Some(project_folder) = common::parse_option_value(args, "--projectFolder") { - return Some(project_folder); - } - for arg in args.iter().rev() { - if !arg.starts_with('-') { - return Some(arg.clone()); - } + return project_folder; } - None + crate::cli::command_positionals("features test", args) + .into_iter() + .next() + .unwrap_or_else(|| ".".to_string()) } #[cfg(test)] @@ -311,20 +306,17 @@ mod tests { let positional = vec!["--quiet".to_string(), "/workspace/project".to_string()]; assert_eq!( - feature_test_project_folder_arg(&dashed).as_deref(), - Some("/workspace/dashed") - ); - assert_eq!( - feature_test_project_folder_arg(&camel).as_deref(), - Some("/workspace/camel") + feature_test_project_folder_arg(&dashed), + "/workspace/dashed" ); + assert_eq!(feature_test_project_folder_arg(&camel), "/workspace/camel"); assert_eq!( - feature_test_project_folder_arg(&positional).as_deref(), - Some("/workspace/project") + feature_test_project_folder_arg(&positional), + "/workspace/project" ); assert_eq!( feature_test_project_folder_arg(&["--quiet".to_string()]), - None + "." ); } diff --git a/cmd/devcontainer/src/commands/collections/mod.rs b/cmd/devcontainer/src/commands/collections/mod.rs index 6650fe85a..dbbaa4112 100644 --- a/cmd/devcontainer/src/commands/collections/mod.rs +++ b/cmd/devcontainer/src/commands/collections/mod.rs @@ -23,19 +23,22 @@ pub(crate) fn run_features(args: &[String]) -> ExitCode { features::build_features_resolve_dependencies_payload(subcommand_args) } "info" => { - if args.len() < 3 { + let positionals = crate::cli::command_positionals("features info", subcommand_args); + if positionals.len() < 2 { Err("features info requires manifest ".to_string()) } else { - let _ = common::parse_option_value(&args[3..], "--log-level"); - let workspace_folder = common::parse_option_value(&args[3..], "--workspace-folder") - .map(std::path::PathBuf::from); + let _ = common::parse_option_value(subcommand_args, "--log-level"); + let workspace_folder = + common::parse_option_value(subcommand_args, "--workspace-folder") + .map(std::path::PathBuf::from); match features::build_feature_info_payload_with_workspace( - &args[1], - &args[2], + &positionals[0], + &positionals[1], workspace_folder.as_deref(), ) { Ok(payload) - if common::parse_option_value(&args[3..], "--output-format").as_deref() + if common::parse_option_value(subcommand_args, "--output-format") + .as_deref() == Some("text") => { println!("{}", render_collection_info_text(&payload)); @@ -47,11 +50,12 @@ pub(crate) fn run_features(args: &[String]) -> ExitCode { } "test" => return feature_tests::run_features_test(subcommand_args), "package" => { - if args.len() < 2 { + let positionals = crate::cli::command_positionals("features package", subcommand_args); + if positionals.is_empty() { Err("features package requires ".to_string()) } else { match publish::package_collection_target( - std::path::Path::new(&args[1]), + std::path::Path::new(&positionals[0]), "devcontainer-feature.json", "feature", ) { @@ -65,33 +69,36 @@ pub(crate) fn run_features(args: &[String]) -> ExitCode { } } "publish" => { - if args.len() < 2 { + let positionals = crate::cli::command_positionals("features publish", subcommand_args); + if positionals.is_empty() { Err("features publish requires ".to_string()) } else { publish::publish_collection_target_to_oci( - std::path::Path::new(&args[1]), + std::path::Path::new(&positionals[0]), "devcontainer-feature.json", "feature", "features publish", - &args[2..], + subcommand_args, ) } } "generate-docs" => { - if args.len() < 2 { + let positionals = + crate::cli::command_positionals("features generate-docs", subcommand_args); + if positionals.is_empty() { Err("features generate-docs requires ".to_string()) } else { let options = common::ManifestDocOptions { registry: Some( - common::parse_option_value(&args[2..], "--registry") + common::parse_option_value(subcommand_args, "--registry") .unwrap_or("ghcr.io".to_string()), ), - namespace: common::parse_option_value(&args[2..], "--namespace"), - github_owner: common::parse_option_value(&args[2..], "--github-owner"), - github_repo: common::parse_option_value(&args[2..], "--github-repo"), + namespace: common::parse_option_value(subcommand_args, "--namespace"), + github_owner: common::parse_option_value(subcommand_args, "--github-owner"), + github_repo: common::parse_option_value(subcommand_args, "--github-repo"), }; match crate::commands::common::generate_manifest_docs( - std::path::Path::new(&args[1]), + std::path::Path::new(&positionals[0]), "devcontainer-feature.json", "Feature", &options, @@ -117,42 +124,54 @@ fn render_collection_info_text(payload: &Value) -> String { } pub(crate) fn run_templates(args: &[String]) -> ExitCode { - let subcommand = args.first().map(String::as_str).unwrap_or(""); + let (subcommand, subcommand_args) = match args.split_first() { + Some((subcommand, subcommand_args)) => (subcommand.as_str(), subcommand_args), + None => ("", &[][..]), + }; let result = match subcommand { - "apply" => templates::run_template_apply(&args[1..]), + "apply" => templates::run_template_apply(subcommand_args), "metadata" => { - if args.len() < 2 { + let positionals = + crate::cli::command_positionals("templates metadata", subcommand_args); + if positionals.is_empty() { Err("templates metadata requires ".to_string()) } else { - let workspace_folder = common::parse_option_value(&args[2..], "--workspace-folder") - .map(std::path::PathBuf::from); - templates::build_template_metadata_payload(&args[1], workspace_folder.as_deref()) + let workspace_folder = + common::parse_option_value(subcommand_args, "--workspace-folder") + .map(std::path::PathBuf::from); + templates::build_template_metadata_payload( + &positionals[0], + workspace_folder.as_deref(), + ) } } "publish" => { - if args.len() < 2 { + let positionals = crate::cli::command_positionals("templates publish", subcommand_args); + if positionals.is_empty() { Err("templates publish requires ".to_string()) } else { publish::publish_collection_target_to_oci( - std::path::Path::new(&args[1]), + std::path::Path::new(&positionals[0]), "devcontainer-template.json", "template", "templates publish", - &args[2..], + subcommand_args, ) } } "generate-docs" => { - if args.len() < 2 { + let positionals = + crate::cli::command_positionals("templates generate-docs", subcommand_args); + if positionals.is_empty() { Err("templates generate-docs requires ".to_string()) } else { let options = common::ManifestDocOptions { - github_owner: common::parse_option_value(&args[2..], "--github-owner"), - github_repo: common::parse_option_value(&args[2..], "--github-repo"), + github_owner: common::parse_option_value(subcommand_args, "--github-owner"), + github_repo: common::parse_option_value(subcommand_args, "--github-repo"), ..Default::default() }; match crate::commands::common::generate_manifest_docs( - std::path::Path::new(&args[1]), + std::path::Path::new(&positionals[0]), "devcontainer-template.json", "Template", &options, diff --git a/cmd/devcontainer/src/commands/collections/templates.rs b/cmd/devcontainer/src/commands/collections/templates.rs index 8eb67e4ad..b719a1f93 100644 --- a/cmd/devcontainer/src/commands/collections/templates.rs +++ b/cmd/devcontainer/src/commands/collections/templates.rs @@ -82,7 +82,8 @@ pub(super) fn run_template_apply(args: &[String]) -> Result { ); } - let target = match args.first() { + let positionals = crate::cli::command_positionals("templates apply", args); + let target = match positionals.first() { Some(target) => target, None => return Err("templates apply requires ".to_string()), }; diff --git a/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs b/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs index 806e4c141..b8115430e 100644 --- a/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs +++ b/cmd/devcontainer/src/commands/collections/tests/feature_tests.rs @@ -294,10 +294,10 @@ fn features_test_discovery_handles_non_object_scenarios_and_missing_test_root() } #[test] -fn features_test_discovery_reports_missing_project_folder_argument() { - let error = discover_feature_test_scenarios(&[]).expect_err("missing project folder"); +fn features_test_discovery_defaults_to_current_project_folder() { + let scenarios = discover_feature_test_scenarios(&[]).expect("default project folder"); - assert_eq!(error, "features test requires a project folder"); + assert!(scenarios.is_empty()); } #[test] @@ -370,9 +370,24 @@ fn features_test_discovery_reports_invalid_scenarios_json() { let _ = fs::remove_dir_all(root); } +#[test] +fn features_test_run_accepts_default_project_folder() { + assert_eq!(run_features_test(&[]), std::process::ExitCode::SUCCESS); +} + #[test] fn features_test_run_reports_discovery_errors() { - assert_eq!(run_features_test(&[]), std::process::ExitCode::from(1)); + let root = unique_temp_dir(); + let test = root.join("test").join("demo"); + fs::create_dir_all(&test).expect("feature test"); + fs::write(test.join("scenarios.json"), "{").expect("invalid scenarios"); + + assert_eq!( + run_features_test(&["--project-folder".to_string(), root.display().to_string(),]), + std::process::ExitCode::from(1) + ); + + let _ = fs::remove_dir_all(root); } #[test] diff --git a/cmd/devcontainer/src/commands/collections/tests/mod.rs b/cmd/devcontainer/src/commands/collections/tests/mod.rs index 6a62b5833..58a8c83c9 100644 --- a/cmd/devcontainer/src/commands/collections/tests/mod.rs +++ b/cmd/devcontainer/src/commands/collections/tests/mod.rs @@ -63,13 +63,17 @@ fn collection_entrypoints_run_package_publish_and_docs_paths() { .expect("feature manifest"); assert_eq!( - super::run_features(&["package".to_string(), root.display().to_string()]), + super::run_features(&[ + "package".to_string(), + "--output-folder".to_string(), + feature_output.display().to_string(), + root.display().to_string(), + ]), ExitCode::SUCCESS ); assert_eq!( super::run_features(&[ "generate-docs".to_string(), - root.display().to_string(), "--registry".to_string(), "ghcr.io".to_string(), "--namespace".to_string(), @@ -78,6 +82,7 @@ fn collection_entrypoints_run_package_publish_and_docs_paths() { "acme".to_string(), "--github-repo".to_string(), "features".to_string(), + root.display().to_string(), ]), ExitCode::SUCCESS ); @@ -85,9 +90,9 @@ fn collection_entrypoints_run_package_publish_and_docs_paths() { assert_eq!( super::run_features(&[ "publish".to_string(), - root.display().to_string(), "--output-dir".to_string(), feature_output.display().to_string(), + root.display().to_string(), ]), ExitCode::SUCCESS ); @@ -128,12 +133,12 @@ fn feature_info_entrypoint_supports_text_output() { assert_eq!( super::run_features(&[ "info".to_string(), - "manifest".to_string(), - root.display().to_string(), "--output-format".to_string(), "text".to_string(), "--log-level".to_string(), "trace".to_string(), + "manifest".to_string(), + root.display().to_string(), ]), ExitCode::SUCCESS ); @@ -194,17 +199,22 @@ fn template_entrypoints_run_metadata_publish_and_docs_paths() { .expect("template manifest"); assert_eq!( - super::run_templates(&["metadata".to_string(), root.display().to_string()]), + super::run_templates(&[ + "metadata".to_string(), + "--log-level".to_string(), + "trace".to_string(), + root.display().to_string(), + ]), ExitCode::SUCCESS ); assert_eq!( super::run_templates(&[ "generate-docs".to_string(), - root.display().to_string(), "--github-owner".to_string(), "acme".to_string(), "--github-repo".to_string(), "templates".to_string(), + root.display().to_string(), ]), ExitCode::SUCCESS ); @@ -212,9 +222,9 @@ fn template_entrypoints_run_metadata_publish_and_docs_paths() { assert_eq!( super::run_templates(&[ "publish".to_string(), - root.display().to_string(), "--output-dir".to_string(), output.display().to_string(), + root.display().to_string(), ]), ExitCode::SUCCESS ); diff --git a/cmd/devcontainer/src/commands/collections/tests/templates.rs b/cmd/devcontainer/src/commands/collections/tests/templates.rs index 37698b7c1..05bba551c 100644 --- a/cmd/devcontainer/src/commands/collections/tests/templates.rs +++ b/cmd/devcontainer/src/commands/collections/tests/templates.rs @@ -210,13 +210,13 @@ fn template_apply_supports_omit_paths_and_tmp_dir() { .expect("failed to write omitted file"); run_template_apply(&[ - template_root.display().to_string(), "--workspace-folder".to_string(), workspace_root.display().to_string(), "--omit-paths".to_string(), "[\".github/*\"]".to_string(), "--tmp-dir".to_string(), tmp_dir.display().to_string(), + template_root.display().to_string(), ]) .expect("apply template"); diff --git a/cmd/devcontainer/src/commands/common.rs b/cmd/devcontainer/src/commands/common.rs index 8567aa030..7bbe083a0 100644 --- a/cmd/devcontainer/src/commands/common.rs +++ b/cmd/devcontainer/src/commands/common.rs @@ -10,11 +10,12 @@ mod manifest; pub(crate) use args::DEVCONTAINER_WORKSPACE_MOUNT_CONSISTENCY; pub(crate) use args::{ config_option_value, env_default_bool_option, env_default_option_value, has_flag, - parse_json_string_array_option, parse_option_value, parse_option_values, remote_env_overrides, - runtime_options, runtime_process_request, secrets_env, validate_choice_option, - validate_number_option, validate_option_values, validate_paired_options, - validate_runtime_env_defaults, DEVCONTAINER_DOCKER_COMPOSE_PATH, DEVCONTAINER_DOCKER_PATH, - DEVCONTAINER_MOUNT_GIT_WORKTREE_COMMON_DIR, DEVCONTAINER_MOUNT_WORKSPACE_GIT_ROOT, + parse_array_option_values, parse_json_string_array_option, parse_option_value, + parse_option_values, remote_env_overrides, runtime_options, runtime_process_request, + secrets_env, validate_choice_option, validate_number_option, validate_option_values, + validate_paired_options, validate_runtime_env_defaults, DEVCONTAINER_DOCKER_COMPOSE_PATH, + DEVCONTAINER_DOCKER_PATH, DEVCONTAINER_MOUNT_GIT_WORKTREE_COMMON_DIR, + DEVCONTAINER_MOUNT_WORKSPACE_GIT_ROOT, }; #[cfg(test)] pub(crate) use args::{ diff --git a/cmd/devcontainer/src/commands/common/args.rs b/cmd/devcontainer/src/commands/common/args.rs index fe93d78e4..cd2bef8dd 100644 --- a/cmd/devcontainer/src/commands/common/args.rs +++ b/cmd/devcontainer/src/commands/common/args.rs @@ -87,11 +87,20 @@ pub(crate) struct RuntimeOptions { } pub(crate) fn parse_option_value(args: &[String], option: &str) -> Option { - args.windows(2) + args_before_separator(args) + .windows(2) .find(|window| window[0] == option) .map(|window| window[1].clone()) } +fn args_before_separator(args: &[String]) -> &[String] { + let end = args + .iter() + .position(|arg| arg == "--") + .unwrap_or(args.len()); + &args[..end] +} + pub(crate) fn env_default_option_value( args: &[String], option: &str, @@ -343,6 +352,7 @@ pub(crate) fn parse_bool_option(args: &[String], option: &str, default: bool) -> const FALSE_VALUES: &[&str] = &["false", "0", "no", "off"]; const TRUE_VALUES: &[&str] = &["true", "1", "yes", "on"]; + let args = args_before_separator(args); let Some(index) = args.iter().position(|arg| arg == option) else { return default; }; @@ -356,6 +366,7 @@ pub(crate) fn parse_bool_option(args: &[String], option: &str, default: bool) -> } pub(crate) fn validate_option_values(args: &[String], options: &[&str]) -> Result<(), String> { + let args = args_before_separator(args); for (index, arg) in args.iter().enumerate() { if options.contains(&arg.as_str()) && args @@ -420,6 +431,7 @@ pub(crate) fn validate_paired_options( } pub(crate) fn parse_option_values(args: &[String], option: &str) -> Vec { + let args = args_before_separator(args); let mut values = Vec::new(); let mut index = 0; while index < args.len() { @@ -433,6 +445,24 @@ pub(crate) fn parse_option_values(args: &[String], option: &str) -> Vec values } +pub(crate) fn parse_array_option_values(args: &[String], option: &str) -> Vec { + let args = args_before_separator(args); + let mut values = Vec::new(); + let mut index = 0; + while index < args.len() { + if args[index] != option { + index += 1; + continue; + } + index += 1; + while args.get(index).is_some_and(|value| !value.starts_with('-')) { + values.push(args[index].clone()); + index += 1; + } + } + values +} + pub(crate) fn parse_json_string_array_option( args: &[String], option: &str, @@ -456,7 +486,7 @@ pub(crate) fn parse_json_string_array_option( } pub(crate) fn has_flag(args: &[String], flag: &str) -> bool { - args.iter().any(|arg| arg == flag) + args_before_separator(args).iter().any(|arg| arg == flag) } pub(crate) fn parse_remote_env(args: &[String]) -> Map { @@ -515,8 +545,9 @@ mod tests { use crate::test_support::unique_temp_dir; use super::{ - env_default_bool_option, env_default_choice_value, env_default_option_value, - parse_bool_option, parse_json_string_array_option, parse_remote_env, remote_env_overrides, + env_default_bool_option, env_default_choice_value, env_default_option_value, has_flag, + parse_array_option_values, parse_bool_option, parse_json_string_array_option, + parse_option_value, parse_option_values, parse_remote_env, remote_env_overrides, runtime_options, runtime_process_request, secrets_env, test_env_defaults, validate_choice_option, validate_number_option, validate_option_values, validate_paired_options, validate_runtime_env_defaults, DEVCONTAINER_BUILDKIT, @@ -527,6 +558,34 @@ mod tests { DEVCONTAINER_USER_DATA_FOLDER, DEVCONTAINER_WORKSPACE_MOUNT_CONSISTENCY, }; + #[test] + fn option_helpers_stop_at_the_separator() { + let args = vec![ + "--before".to_string(), + "value".to_string(), + "--array".to_string(), + "one".to_string(), + "two".to_string(), + "--".to_string(), + "--after".to_string(), + "payload".to_string(), + ]; + + assert_eq!( + parse_option_value(&args, "--before").as_deref(), + Some("value") + ); + assert_eq!(parse_option_values(&args, "--before"), vec!["value"]); + assert_eq!( + parse_array_option_values(&args, "--array"), + vec!["one", "two"] + ); + assert!(has_flag(&args, "--before")); + assert_eq!(parse_option_value(&args, "--after"), None); + assert!(!has_flag(&args, "--after")); + assert!(validate_option_values(&args, &["--after"]).is_ok()); + } + #[test] fn runtime_options_collect_shared_runtime_flags() { let options = runtime_options(&[ diff --git a/cmd/devcontainer/src/lib.rs b/cmd/devcontainer/src/lib.rs index 06979e655..325cec9a2 100644 --- a/cmd/devcontainer/src/lib.rs +++ b/cmd/devcontainer/src/lib.rs @@ -229,5 +229,15 @@ mod tests { ]), ExitCode::from(2) ); + assert_eq!( + run(vec![ + "--log-format".to_string(), + "json".to_string(), + "read-configuration".to_string(), + "--user-data-folder".to_string(), + "/tmp/devcontainer-user-data".to_string(), + ]), + ExitCode::from(2) + ); } } diff --git a/cmd/devcontainer/src/runtime/build.rs b/cmd/devcontainer/src/runtime/build.rs index 7e3b38737..470bbc4e7 100644 --- a/cmd/devcontainer/src/runtime/build.rs +++ b/cmd/devcontainer/src/runtime/build.rs @@ -29,6 +29,7 @@ pub(crate) fn runtime_image_name( } else if has_build_definition(&resolved.configuration) || has_native_features { build_image(resolved, args) } else if let Some(image) = resolved.configuration.get("image").and_then(Value::as_str) { + pull_source_image_if_requested(args, image)?; Ok(image.to_string()) } else { Err( @@ -71,8 +72,13 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< ); lockfile_validation?; let image_name = image_name_arg_or_default(args, &resolved.workspace_folder); - let built = - build_feature_image(args, &image_name, &image, &feature_support.installations)?; + let built = build_feature_image( + args, + &image_name, + &image, + &feature_support.installations, + engine::pull_always_requested(args), + )?; maybe_push_image(args, &built)?; let lockfile_update = configuration::ensure_native_lockfile( args, @@ -83,6 +89,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< lockfile_update?; Ok(built) } else { + pull_source_image_if_requested(args, &image)?; Ok(image) }; } @@ -99,7 +106,7 @@ pub(crate) fn build_image(resolved: &ResolvedConfig, args: &[String]) -> Result< let base_image = format!("{image_name}-base"); build_base_image(resolved, args, &base_image)?; let installations = &feature_support.installations; - let built = build_feature_image(args, &image_name, &base_image, installations)?; + let built = build_feature_image(args, &image_name, &base_image, installations, false)?; maybe_push_image(args, &built)?; let lockfile_update = configuration::ensure_native_lockfile( args, @@ -139,6 +146,9 @@ fn build_base_image( let dockerfile_path = resolve_relative(config_root, dockerfile); let context_path = resolve_relative(config_root, context); let mut engine_args = engine_build_args(args, image_name, &dockerfile_path); + if engine::pull_always_requested(args) { + engine_args.push("--pull".to_string()); + } if let Some(build_args) = build.get("args").and_then(Value::as_object) { for (key, value) in build_args { if let Some(value) = value.as_str() { @@ -162,12 +172,16 @@ pub(crate) fn build_feature_image( image_name: &str, base_image: &str, installations: &[configuration::FeatureInstallation], + pull_base_image: bool, ) -> Result { let build_context_dir = unique_feature_build_dir(); fs::create_dir_all(&build_context_dir).map_err(|error| error.to_string())?; let dockerfile_path = write_feature_dockerfile(args, &build_context_dir, base_image, installations)?; let mut engine_args = engine_build_args(args, image_name, &dockerfile_path); + if pull_base_image { + engine_args.push("--pull".to_string()); + } engine_args.push(build_context_dir.display().to_string()); let result = engine::run_engine(args, engine_args); @@ -180,6 +194,17 @@ pub(crate) fn build_feature_image( Ok(image_name.to_string()) } +fn pull_source_image_if_requested(args: &[String], image_name: &str) -> Result<(), String> { + if !engine::pull_always_requested(args) { + return Ok(()); + } + let result = engine::run_engine(args, vec!["pull".to_string(), image_name.to_string()])?; + if result.status_code != 0 { + return Err(engine::stderr_or_stdout(&result)); + } + Ok(()) +} + fn maybe_push_image(args: &[String], image_name: &str) -> Result<(), String> { if !common::has_flag(args, "--push") { return Ok(()); @@ -550,6 +575,173 @@ mod tests { ); } + #[test] + fn runtime_image_name_pull_always_refreshes_plain_source_images() { + let root = unique_temp_dir("devcontainer-runtime-image-name-pull"); + let resolved = resolved_config( + &root, + json!({ + "image": "debian:bookworm" + }), + ); + let log = root.join("engine.log"); + let engine = write_engine_script( + &root, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nexit 0\n", + log.display() + ), + ); + let args = vec![ + "--docker-path".to_string(), + engine.display().to_string(), + "--pull-always=true".to_string(), + ]; + + assert_eq!( + runtime_image_name(&resolved, &args).expect("image name"), + "debian:bookworm" + ); + assert_eq!( + fs::read_to_string(log).expect("engine log"), + "pull debian:bookworm\n" + ); + } + + #[test] + fn runtime_image_name_reports_pull_failures_for_plain_source_images() { + let root = unique_temp_dir("devcontainer-runtime-image-name-pull-failure"); + let resolved = resolved_config( + &root, + json!({ + "image": "debian:bookworm" + }), + ); + let engine = write_engine_script( + &root, + "#!/bin/sh\necho source image pull failed >&2\nexit 23\n", + ); + + let error = runtime_image_name( + &resolved, + &[ + "--docker-path".to_string(), + engine.display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect_err("source image pull failure"); + + assert_eq!(error, "source image pull failed"); + } + + #[test] + fn pull_always_refreshes_remote_build_bases_without_pulling_generated_feature_tags() { + let root = unique_temp_dir("devcontainer-build-feature-pull"); + write_local_feature(&root, "local-feature", json!({})); + let resolved = resolved_config( + &root, + json!({ + "build": { "dockerfile": "Dockerfile" }, + "features": { "../local-feature": {} } + }), + ); + fs::write( + root.join(".devcontainer").join("Dockerfile"), + "FROM debian:bookworm\n", + ) + .expect("dockerfile"); + let log = root.join("engine.log"); + let engine = write_engine_script( + &root, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nexit 0\n", + log.display() + ), + ); + let args = vec![ + "--docker-path".to_string(), + engine.display().to_string(), + "--image-name".to_string(), + "example/featured:test".to_string(), + "--pull-always".to_string(), + ]; + + assert_eq!( + build_image(&resolved, &args).expect("featured image"), + "example/featured:test" + ); + let log = fs::read_to_string(log).expect("engine log"); + let builds = log.lines().collect::>(); + assert_eq!(builds.len(), 2, "{log}"); + assert!(builds[0].contains("--pull"), "{log}"); + assert!(!builds[1].contains("--pull"), "{log}"); + } + + #[test] + fn pull_always_refreshes_remote_image_under_feature_builds() { + let root = unique_temp_dir("devcontainer-image-feature-pull"); + write_local_feature(&root, "local-feature", json!({})); + let resolved = resolved_config( + &root, + json!({ + "image": "debian:bookworm", + "features": { "../local-feature": {} } + }), + ); + let log = root.join("engine.log"); + let engine = write_engine_script( + &root, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nexit 0\n", + log.display() + ), + ); + let args = vec![ + "--docker-path".to_string(), + engine.display().to_string(), + "--image-name".to_string(), + "example/featured:test".to_string(), + "--pull-always".to_string(), + ]; + + build_image(&resolved, &args).expect("feature image"); + let log = fs::read_to_string(log).expect("engine log"); + assert_eq!(log.lines().count(), 1, "{log}"); + assert!(log.contains("build --tag example/featured:test"), "{log}"); + assert!(log.contains("--pull"), "{log}"); + } + + #[test] + fn build_image_propagates_feature_build_failures_for_image_configs() { + let root = unique_temp_dir("devcontainer-image-feature-build-failure"); + write_local_feature(&root, "local-feature", json!({})); + let resolved = resolved_config( + &root, + json!({ + "image": "debian:bookworm", + "features": { "../local-feature": {} } + }), + ); + let engine = write_engine_script( + &root, + "#!/bin/sh\necho feature image build failed >&2\nexit 17\n", + ); + + let error = build_image( + &resolved, + &[ + "--docker-path".to_string(), + engine.display().to_string(), + "--image-name".to_string(), + "example/featured:test".to_string(), + ], + ) + .expect_err("feature image build failure"); + + assert_eq!(error, "feature image build failed"); + } + #[test] fn runtime_image_name_reports_unsupported_configs() { let root = unique_temp_dir("devcontainer-runtime-image-name-unsupported"); @@ -1030,8 +1222,14 @@ exit 0 ); let args = vec!["--docker-path".to_string(), engine.display().to_string()]; - let image = build_feature_image(&args, "example/native:test", "example/base:test", &[]) - .expect("feature build"); + let image = build_feature_image( + &args, + "example/native:test", + "example/base:test", + &[], + false, + ) + .expect("feature build"); assert_eq!(image, "example/native:test"); let log = fs::read_to_string(log).expect("engine log"); @@ -1082,8 +1280,14 @@ exit 0 ); let args = vec!["--docker-path".to_string(), engine.display().to_string()]; - let error = build_feature_image(&args, "example/native:test", "example/base:test", &[]) - .expect_err("feature build failure"); + let error = build_feature_image( + &args, + "example/native:test", + "example/base:test", + &[], + false, + ) + .expect_err("feature build failure"); assert_eq!(error, "feature build rejected"); } @@ -1098,8 +1302,14 @@ exit 0 missing_engine.display().to_string(), ]; - let error = build_feature_image(&args, "example/native:test", "example/base:test", &[]) - .expect_err("feature build spawn failure"); + let error = build_feature_image( + &args, + "example/native:test", + "example/base:test", + &[], + false, + ) + .expect_err("feature build spawn failure"); assert!(error.contains("missing-engine"), "{error}"); let _ = fs::remove_dir_all(root); diff --git a/cmd/devcontainer/src/runtime/compose/mod.rs b/cmd/devcontainer/src/runtime/compose/mod.rs index 38c9c552d..13313d375 100644 --- a/cmd/devcontainer/src/runtime/compose/mod.rs +++ b/cmd/devcontainer/src/runtime/compose/mod.rs @@ -5,15 +5,18 @@ mod override_file; mod project; mod service; -use std::path::PathBuf; +use std::collections::HashSet; +use std::path::{Path, PathBuf}; use serde_json::Value; +use serde_yaml::{Mapping, Value as YamlValue}; use crate::commands::common; use crate::commands::configuration; use super::context::ResolvedConfig; use super::engine; +use super::paths::unique_temp_path; const COMPOSE_PROJECT_LABEL: &str = "com.docker.compose.project"; const COMPOSE_SERVICE_LABEL: &str = "com.docker.compose.service"; @@ -99,6 +102,10 @@ pub(crate) fn build_service(resolved: &ResolvedConfig, args: &[String]) -> Resul )?; } + if engine::pull_always_requested(args) { + pull_compose_images(resolved, args, &spec)?; + } + if spec.has_build { let build_override_file = override_file::compose_build_override_file(&spec, args)?; let mut build_args = vec!["--pull".to_string()]; @@ -127,7 +134,13 @@ pub(crate) fn build_service(resolved: &ResolvedConfig, args: &[String]) -> Resul let built_image = common::parse_option_value(args, "--image-name") .unwrap_or_else(|| compose_image.clone()); let installations = &feature_support.installations; - super::build::build_feature_image(args, &built_image, &compose_image, installations)?; + super::build::build_feature_image( + args, + &built_image, + &compose_image, + installations, + false, + )?; let configuration = &resolved.configuration; configuration::ensure_native_lockfile( args, @@ -153,7 +166,9 @@ pub(crate) fn up_service( ) -> Result { let spec = load_compose_spec(resolved)? .ok_or_else(|| "Compose configuration was expected but not found".to_string())?; - let override_file = override_file::compose_metadata_override_file( + let selected_services = compose_services_to_start(resolved, &spec); + let profile_override_file = compose_profile_override_file(&spec, selected_services.as_deref())?; + let override_file = match override_file::compose_metadata_override_file( resolved, args, remote_workspace_folder, @@ -162,36 +177,28 @@ pub(crate) fn up_service( } else { None }, - )?; + ) { + Ok(override_file) => override_file, + Err(error) => { + remove_compose_override(profile_override_file.as_ref()); + return Err(error); + } + }; let mut up_args = vec!["-d".to_string()]; - if common::has_flag(args, "--pull-always") { - up_args.extend(engine::compose_pull_always_args(args)); - } if no_recreate { up_args.push("--no-recreate".to_string()); } - if let Some(run_services) = resolved - .configuration - .get("runServices") - .and_then(Value::as_array) - .filter(|services| !services.is_empty()) - { - let mut has_primary_service = false; - for service in run_services.iter().filter_map(Value::as_str) { - has_primary_service |= service == spec.service; - up_args.push(service.to_string()); - } - if !has_primary_service { - up_args.push(spec.service.clone()); - } + if let Some(services) = selected_services { + up_args.extend(services); } - let result = engine::run_compose( - args, - args::compose_args_owned(&spec, "up", override_file.as_ref(), up_args), - ); - if let Some(override_file) = override_file { - let _ = std::fs::remove_file(override_file); + let tail_len = up_args.len(); + let mut compose_args = args::compose_args_owned(&spec, "up", override_file.as_ref(), up_args); + if let Some(profile_override_file) = &profile_override_file { + insert_compose_override(&mut compose_args, tail_len, profile_override_file); } + let result = engine::run_compose(args, compose_args); + remove_compose_override(override_file.as_ref()); + remove_compose_override(profile_override_file.as_ref()); let result = result?; if result.status_code != 0 { return Err(engine::stderr_or_stdout(&result)); @@ -204,6 +211,268 @@ pub(crate) fn up_service( }) } +fn pull_compose_images( + resolved: &ResolvedConfig, + args: &[String], + spec: &ComposeSpec, +) -> Result<(), String> { + let selected_services = compose_services_to_start(resolved, spec); + let profile_override_file = compose_profile_override_file(spec, selected_services.as_deref())?; + let result = pull_compose_images_with_override( + args, + spec, + selected_services.as_deref(), + profile_override_file.as_ref(), + ); + remove_compose_override(profile_override_file.as_ref()); + result +} + +fn pull_compose_images_with_override( + args: &[String], + spec: &ComposeSpec, + selected_services: Option<&[String]>, + profile_override_file: Option<&PathBuf>, +) -> Result<(), String> { + let config_args = args::compose_args_owned(spec, "config", profile_override_file, Vec::new()); + let result = engine::run_compose(args, config_args)?; + if result.status_code != 0 { + return Err(engine::stderr_or_stdout(&result)); + } + let compose_config: YamlValue = serde_yaml::from_str(&result.stdout) + .map_err(|error| format!("Unable to parse resolved Compose configuration: {error}"))?; + let services = compose_config + .as_mapping() + .and_then(|root| yaml_field(root, "services")) + .and_then(YamlValue::as_mapping) + .ok_or_else(|| "Resolved Compose configuration does not define services".to_string())?; + let services_to_pull = remote_service_closure(services, selected_services)?; + let mut pulled_images = HashSet::new(); + for service in services_to_pull { + let definition = services + .get(YamlValue::String(service)) + .and_then(YamlValue::as_mapping) + .expect("remote service closure only returns defined services"); + let image = yaml_field(definition, "image") + .and_then(YamlValue::as_str) + .expect("remote service closure only returns services with images"); + let platform = yaml_field(definition, "platform") + .and_then(YamlValue::as_str) + .filter(|platform| !platform.trim().is_empty()); + if !pulled_images.insert((image.to_string(), platform.map(str::to_string))) { + continue; + } + let mut pull_args = vec!["pull".to_string()]; + if let Some(platform) = platform { + pull_args.extend(["--platform".to_string(), platform.to_string()]); + } + pull_args.push(image.to_string()); + let result = engine::run_engine(args, pull_args)?; + if result.status_code != 0 { + return Err(engine::stderr_or_stdout(&result)); + } + } + Ok(()) +} + +fn compose_profile_override_file( + spec: &ComposeSpec, + selected_services: Option<&[String]>, +) -> Result, String> { + if selected_services.is_none() { + return Ok(None); + } + let profiled_services = compose_profiled_services(spec)?; + if profiled_services.is_empty() { + return Ok(None); + } + + let mut content = service::read_version_prefix(&spec.files)?; + content.push_str("services:\n"); + for service in profiled_services { + content.push_str(&format!( + " '{}':\n profiles: !reset []\n", + service.replace('\'', "''") + )); + } + let path = unique_temp_path("devcontainer-compose-profile-override", Some("yml")); + std::fs::write(&path, content).map_err(|error| error.to_string())?; + Ok(Some(path)) +} + +fn compose_profiled_services(spec: &ComposeSpec) -> Result, String> { + let mut profiled_services = HashSet::new(); + for compose_file in &spec.files { + let raw = std::fs::read_to_string(compose_file).map_err(|error| error.to_string())?; + let parsed: YamlValue = serde_yaml::from_str(&raw).map_err(|error| error.to_string())?; + let Some(services) = parsed + .as_mapping() + .and_then(|root| yaml_field(root, "services")) + .and_then(YamlValue::as_mapping) + else { + continue; + }; + for (service, definition) in services { + let (Some(service), Some(definition)) = (service.as_str(), definition.as_mapping()) + else { + continue; + }; + let Some(profiles) = yaml_field(definition, "profiles") else { + continue; + }; + match compose_profiles_state(profiles) { + Some(true) => { + profiled_services.insert(service.to_string()); + } + Some(false) => { + profiled_services.remove(service); + } + None => {} + } + } + } + let mut profiled_services = profiled_services.into_iter().collect::>(); + profiled_services.sort(); + Ok(profiled_services) +} + +fn compose_profiles_state(profiles: &YamlValue) -> Option { + match profiles { + YamlValue::Sequence(profiles) if !profiles.is_empty() => Some(true), + YamlValue::Sequence(_) => None, + YamlValue::Tagged(tagged) if tagged.tag == "!reset" => { + Some(compose_profiles_state(&tagged.value).unwrap_or(false)) + } + YamlValue::Tagged(tagged) => compose_profiles_state(&tagged.value), + YamlValue::Null => Some(false), + _ => None, + } +} + +fn insert_compose_override(args: &mut Vec, tail_len: usize, override_file: &Path) { + let subcommand_index = args.len() - tail_len - 1; + args.splice( + subcommand_index..subcommand_index, + ["-f".to_string(), override_file.display().to_string()], + ); +} + +fn remove_compose_override(override_file: Option<&PathBuf>) { + if let Some(override_file) = override_file { + let _ = std::fs::remove_file(override_file); + } +} + +fn compose_services_to_start(resolved: &ResolvedConfig, spec: &ComposeSpec) -> Option> { + let run_services = resolved + .configuration + .get("runServices") + .and_then(Value::as_array) + .filter(|services| !services.is_empty())?; + let mut services = run_services + .iter() + .filter_map(Value::as_str) + .map(str::to_string) + .collect::>(); + if !services.iter().any(|service| service == &spec.service) { + services.push(spec.service.clone()); + } + Some(services) +} + +fn remote_service_closure( + services: &Mapping, + selected_services: Option<&[String]>, +) -> Result, String> { + let roots = match selected_services { + Some(services) => services.to_vec(), + None => services + .keys() + .filter_map(YamlValue::as_str) + .map(str::to_string) + .collect(), + }; + let mut visited = HashSet::new(); + let mut remote_services = Vec::new(); + for service in roots { + visit_remote_service(services, &service, &mut visited, &mut remote_services)?; + } + Ok(remote_services) +} + +fn visit_remote_service( + services: &Mapping, + service: &str, + visited: &mut HashSet, + remote_services: &mut Vec, +) -> Result<(), String> { + if !visited.insert(service.to_string()) { + return Ok(()); + } + let definition = services + .get(YamlValue::String(service.to_string())) + .and_then(YamlValue::as_mapping) + .ok_or_else(|| { + format!("Unable to locate compose service `{service}` in resolved configuration") + })?; + for dependency in compose_service_dependencies(definition) { + visit_remote_service(services, &dependency, visited, remote_services)?; + } + + let has_remote_image = yaml_field(definition, "image") + .and_then(YamlValue::as_str) + .is_some_and(|image| !image.trim().is_empty()); + let has_build = yaml_field(definition, "build").is_some_and(|build| !build.is_null()); + if has_remote_image && !has_build { + remote_services.push(service.to_string()); + } + Ok(()) +} + +fn compose_service_dependencies(definition: &Mapping) -> Vec { + let mut dependencies = Vec::new(); + if let Some(depends_on) = yaml_field(definition, "depends_on") { + match depends_on { + YamlValue::Mapping(mapping) => dependencies.extend( + mapping + .keys() + .filter_map(YamlValue::as_str) + .map(str::to_string), + ), + YamlValue::Sequence(sequence) => dependencies.extend( + sequence + .iter() + .filter_map(YamlValue::as_str) + .map(str::to_string), + ), + _ => {} + } + } + for field in ["links", "volumes_from"] { + if let Some(values) = yaml_field(definition, field).and_then(YamlValue::as_sequence) { + dependencies.extend(values.iter().filter_map(YamlValue::as_str).filter_map( + |reference| { + let service = reference.split(':').next()?; + (service != "container" && !service.is_empty()).then(|| service.to_string()) + }, + )); + } + } + for field in ["ipc", "network_mode", "pid"] { + if let Some(service) = yaml_field(definition, field) + .and_then(YamlValue::as_str) + .and_then(|reference| reference.strip_prefix("service:")) + { + dependencies.push(service.to_string()); + } + } + dependencies +} + +fn yaml_field<'a>(mapping: &'a Mapping, field: &str) -> Option<&'a YamlValue> { + mapping.get(YamlValue::String(field.to_string())) +} + pub(crate) fn service_logs( resolved: &ResolvedConfig, args: &[String], diff --git a/cmd/devcontainer/src/runtime/compose/tests.rs b/cmd/devcontainer/src/runtime/compose/tests.rs index 23e6745c0..bf195d6b6 100644 --- a/cmd/devcontainer/src/runtime/compose/tests.rs +++ b/cmd/devcontainer/src/runtime/compose/tests.rs @@ -1,6 +1,7 @@ //! Unit tests for compose runtime helpers. use serde_json::json; +use serde_yaml::Value as YamlValue; use std::env; use std::fs; use std::path::PathBuf; @@ -16,8 +17,9 @@ use super::service::{ }; use super::uses_compose_config; use super::{ - build_service, load_compose_spec, resolve_container_id, resolve_container_id_including_stopped, - up_service, ComposeSpec, + build_service, compose_profiled_services, compose_profiles_state, compose_service_dependencies, + load_compose_spec, remote_service_closure, resolve_container_id, + resolve_container_id_including_stopped, up_service, ComposeSpec, }; use crate::runtime::context::ResolvedConfig; use crate::test_support::{ @@ -1629,6 +1631,216 @@ fn build_service_reports_compose_build_failures() { let _ = fs::remove_dir_all(root); } +#[test] +fn build_service_pull_always_reports_engine_pull_failures() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("workspace root"); + fs::write( + root.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose file"); + let compose_path = root.join("compose.sh"); + write_executable_script( + &compose_path, + "#!/bin/sh\nprintf '%s\\n' 'services:' ' app:' ' image: alpine:3.20'\nexit 0\n", + ); + let engine_path = root.join("engine.sh"); + write_executable_script( + &engine_path, + "#!/bin/sh\necho engine pull failed >&2\nexit 12\n", + ); + let resolved = resolved_config( + root.clone(), + json!({ + "dockerComposeFile": "docker-compose.yml", + "service": "app" + }), + ); + + let error = build_service( + &resolved, + &[ + "--docker-compose-path".to_string(), + compose_path.display().to_string(), + "--docker-path".to_string(), + engine_path.display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect_err("engine pull failure"); + + assert_eq!(error, "engine pull failed"); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn build_service_pull_always_deduplicates_matching_image_and_platform_pulls() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("workspace root"); + fs::write( + root.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose file"); + let compose_path = root.join("compose.sh"); + write_executable_script( + &compose_path, + "#!/bin/sh\nprintf '%s\\n' 'services:' ' app:' ' image: alpine:3.20' ' platform: linux/amd64' ' sidecar:' ' image: alpine:3.20' ' platform: linux/amd64'\nexit 0\n", + ); + let engine_path = root.join("engine.sh"); + let log = root.join("engine.log"); + write_executable_script( + &engine_path, + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nexit 0\n", + log.display() + ), + ); + let resolved = resolved_config( + root.clone(), + json!({ + "dockerComposeFile": "docker-compose.yml", + "service": "app" + }), + ); + + let image = build_service( + &resolved, + &[ + "--docker-compose-path".to_string(), + compose_path.display().to_string(), + "--docker-path".to_string(), + engine_path.display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect("deduplicated image pull"); + + assert_eq!(image, "alpine:3.20"); + assert_eq!( + fs::read_to_string(log).expect("engine log"), + "pull --platform linux/amd64 alpine:3.20\n" + ); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn build_service_pull_always_reports_compose_config_failures() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("workspace root"); + fs::write( + root.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose file"); + let compose_path = root.join("compose.sh"); + write_executable_script( + &compose_path, + "#!/bin/sh\necho compose config failed >&2\nexit 11\n", + ); + let resolved = resolved_config( + root.clone(), + json!({ + "dockerComposeFile": "docker-compose.yml", + "service": "app" + }), + ); + + let error = build_service( + &resolved, + &[ + "--docker-compose-path".to_string(), + compose_path.display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect_err("compose config failure"); + + assert_eq!(error, "compose config failed"); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn build_service_pull_always_reports_engine_binary_disappearing_before_pull() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("workspace root"); + fs::write( + root.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose file"); + let compose_path = root.join("compose.sh"); + let engine_path = root.join("engine.sh"); + write_executable_script(&engine_path, "#!/bin/sh\nexit 0\n"); + write_executable_script( + &compose_path, + &format!( + "#!/bin/sh\nprintf '%s\\n' 'services:' ' app:' ' image: alpine:3.20'\nrm -f '{}'\nexit 0\n", + engine_path.display() + ), + ); + let resolved = resolved_config( + root.clone(), + json!({ + "dockerComposeFile": "docker-compose.yml", + "service": "app" + }), + ); + + let error = build_service( + &resolved, + &[ + "--docker-compose-path".to_string(), + compose_path.display().to_string(), + "--docker-path".to_string(), + engine_path.display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect_err("compose pull spawn failure"); + + assert!( + error.contains("Container engine executable not found"), + "{error}" + ); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn build_service_pull_always_reports_missing_compose_binary() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("workspace root"); + fs::write( + root.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose file"); + let resolved = resolved_config( + root.clone(), + json!({ + "dockerComposeFile": "docker-compose.yml", + "service": "app" + }), + ); + + let error = build_service( + &resolved, + &[ + "--docker-compose-path".to_string(), + root.join("missing-compose").display().to_string(), + "--pull-always".to_string(), + ], + ) + .expect_err("missing compose binary"); + + assert!( + error.contains("Container compose executable not found"), + "{error}" + ); + let _ = fs::remove_dir_all(root); +} + #[test] fn build_service_reports_feature_build_failures() { let root = unique_temp_dir("devcontainer-compose-test"); @@ -1759,7 +1971,7 @@ fn build_service_builds_feature_image_successfully() { write_executable_script( &engine_path, &format!( - "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nexit 0\n", + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\ncase \" $* \" in\n *\" config \"*) printf '%s\\n' 'services:' ' app:' ' image: alpine:3.20' ;;\nesac\nexit 0\n", log.display() ), ); @@ -1781,6 +1993,7 @@ fn build_service_builds_feature_image_successfully() { engine_path.display().to_string(), "--image-name".to_string(), "example/compose-featured:test".to_string(), + "--pull-always".to_string(), ], ) .expect("feature image"); @@ -1791,9 +2004,129 @@ fn build_service_builds_feature_image_successfully() { log.contains("build --tag example/compose-featured:test"), "{log}" ); + let feature_build = log + .lines() + .find(|line| line.starts_with("build --tag example/compose-featured:test")) + .expect("feature build invocation"); + assert!( + !feature_build + .split_whitespace() + .any(|argument| argument == "--pull"), + "{log}" + ); + assert!(log.lines().any(|line| line == "pull alpine:3.20"), "{log}"); + let _ = fs::remove_dir_all(root); +} + +#[test] +fn compose_profile_states_cover_resets_tags_null_and_invalid_shapes() { + let reset: YamlValue = serde_yaml::from_str("!reset []").expect("reset profiles"); + assert_eq!(compose_profiles_state(&reset), Some(false)); + + let tagged: YamlValue = serde_yaml::from_str("!override [debug]").expect("tagged profiles"); + assert_eq!(compose_profiles_state(&tagged), Some(true)); + + assert_eq!( + compose_profiles_state(&YamlValue::Sequence(Vec::new())), + None + ); + assert_eq!(compose_profiles_state(&YamlValue::Null), Some(false)); + assert_eq!( + compose_profiles_state(&YamlValue::String("debug".to_string())), + None + ); +} + +#[test] +fn compose_profiled_services_merge_files_and_ignore_invalid_entries() { + let root = unique_temp_dir("devcontainer-compose-test"); + fs::create_dir_all(&root).expect("compose root"); + let no_services = root.join("no-services.yml"); + let invalid_services = root.join("invalid-services.yml"); + let profiles = root.join("profiles.yml"); + let reset = root.join("reset.yml"); + fs::write(&no_services, "name: ignored\n").expect("no services compose"); + fs::write( + &invalid_services, + "services:\n 1:\n profiles: [debug]\n scalar: alpine:3.20\n", + ) + .expect("invalid services compose"); + fs::write( + &profiles, + "services:\n app:\n profiles: [debug]\n keep:\n profiles: [test]\n empty:\n profiles: []\n scalar-profile:\n profiles: debug\n", + ) + .expect("profiled compose"); + fs::write(&reset, "services:\n app:\n profiles: null\n").expect("profile reset compose"); + let spec = ComposeSpec { + files: vec![no_services, invalid_services, profiles, reset], + service: "app".to_string(), + image: None, + has_build: false, + user: None, + project_name: "project".to_string(), + }; + + assert_eq!( + compose_profiled_services(&spec).expect("profiled services"), + vec!["keep".to_string()] + ); let _ = fs::remove_dir_all(root); } +#[test] +fn remote_service_closure_handles_cycles_and_reports_missing_dependencies() { + let cyclic: YamlValue = serde_yaml::from_str( + "services:\n app:\n image: example/app\n depends_on: [db]\n db:\n image: postgres:16\n depends_on: [app]\n", + ) + .expect("cyclic compose config"); + let services = cyclic + .as_mapping() + .and_then(|root| root.get(YamlValue::String("services".to_string()))) + .and_then(YamlValue::as_mapping) + .expect("services"); + assert_eq!( + remote_service_closure(services, Some(&["app".to_string()])) + .expect("remote service closure"), + vec!["db".to_string(), "app".to_string()] + ); + + let missing: YamlValue = serde_yaml::from_str( + "services:\n app:\n image: example/app\n depends_on: [missing]\n", + ) + .expect("missing dependency compose config"); + let services = missing + .as_mapping() + .and_then(|root| root.get(YamlValue::String("services".to_string()))) + .and_then(YamlValue::as_mapping) + .expect("services"); + let error = remote_service_closure(services, Some(&["app".to_string()])) + .expect_err("missing dependency"); + assert_eq!( + error, + "Unable to locate compose service `missing` in resolved configuration" + ); +} + +#[test] +fn compose_service_dependencies_support_all_compose_reference_shapes() { + let value: YamlValue = serde_yaml::from_str( + "depends_on: invalid\nlinks:\n - cache:alias\n - container:external\n - ''\n - 42\nvolumes_from:\n - worker:ro\nipc: service:ipc-service\nnetwork_mode: service:network-service\npid: service:pid-service\n", + ) + .expect("service definition"); + let definition = value.as_mapping().expect("service mapping"); + + assert_eq!( + compose_service_dependencies(definition), + vec![ + "cache".to_string(), + "worker".to_string(), + "ipc-service".to_string(), + "network-service".to_string(), + "pid-service".to_string(), + ] + ); +} + #[test] fn up_service_pins_rebuilt_images_and_reports_compose_errors() { let root = unique_temp_dir("devcontainer-compose-test"); diff --git a/cmd/devcontainer/src/runtime/container/engine_run.rs b/cmd/devcontainer/src/runtime/container/engine_run.rs index b4a6d5713..b0b62b2f5 100644 --- a/cmd/devcontainer/src/runtime/container/engine_run.rs +++ b/cmd/devcontainer/src/runtime/container/engine_run.rs @@ -59,12 +59,9 @@ fn start_container_with_metadata( let default_labels = common::default_devcontainer_id_labels(&resolved.workspace_folder, &resolved.config_file); let metadata = metadata?; - let mut engine_args = vec!["run".to_string(), "-d".to_string()]; - if common::has_flag(args, "--pull-always") { - engine_args.push("--pull".to_string()); - engine_args.push("always".to_string()); - } - engine_args.extend([ + let mut engine_args = vec![ + "run".to_string(), + "-d".to_string(), "--label".to_string(), default_labels[0].clone(), "--label".to_string(), @@ -73,7 +70,7 @@ fn start_container_with_metadata( format!("devcontainer.metadata={metadata}"), "--mount".to_string(), workspace_mount_for_args(resolved, remote_workspace_folder, args), - ]); + ]; if resolved.configuration.get("workspaceMount").is_none() { for mount in additional_mounts_for_workspace_target(resolved, remote_workspace_folder, args) { @@ -486,13 +483,11 @@ esac }), ); - let container_id = start_container( - &resolved, - &engine_args(&fake_engine), - "alpine:3.20", - "/workspace", - ) - .expect("container should start"); + let mut args = engine_args(&fake_engine); + args.push("--pull-always".to_string()); + let container_id = + start_container(&resolved, &args, "devcontainer-workspace-uid", "/workspace") + .expect("container should start"); assert_eq!(container_id, "created-container"); let invocation = fs::read_to_string(&invocation_log).expect("invocation log"); @@ -500,6 +495,11 @@ esac assert!(invocation.contains("-e EDITOR=vim")); assert!(invocation.contains("--cap-add SYS_PTRACE")); assert!(invocation.contains("--security-opt seccomp=unconfined")); + assert!(!invocation.contains("--pull always"), "{invocation}"); + assert!( + invocation.contains("devcontainer-workspace-uid"), + "{invocation}" + ); let _ = fs::remove_dir_all(root); } diff --git a/cmd/devcontainer/src/runtime/container/uid_update/tests.rs b/cmd/devcontainer/src/runtime/container/uid_update/tests.rs index 631f7eced..4733e9214 100644 --- a/cmd/devcontainer/src/runtime/container/uid_update/tests.rs +++ b/cmd/devcontainer/src/runtime/container/uid_update/tests.rs @@ -272,6 +272,32 @@ fn prepare_up_image_preserves_the_inspected_platform_for_uid_update_builds() { assert!(invocations.contains("--platform linux/arm64/v8")); } +#[test] +fn pull_always_does_not_force_pull_the_generated_uid_image() { + let fixture = FakeEngineFixture::new(); + fixture.write( + "image-inspect.stdout", + &image_inspect_output("node", Some("linux/amd64")), + ); + let workspace = fixture.root.join("workspace"); + fs::create_dir_all(&workspace).expect("workspace dir"); + let resolved = resolved_config(json!({ "remoteUser": "vscode" }), &workspace); + let mut args = fixture.args(); + args.push("--pull-always".to_string()); + + let updated_image = + prepare_up_image_for_platform(&resolved, &args, "ghcr.io/example/app:latest", true) + .expect("prepare up image"); + + assert!(updated_image.ends_with("-uid")); + let invocations = fixture.invocations(); + let build = invocations + .lines() + .find(|line| line.starts_with("build ")) + .expect("uid update build"); + assert!(!build.contains("--pull"), "{invocations}"); +} + #[test] fn prepare_up_image_prefixes_local_podman_base_images_with_localhost() { let fixture = FakeEngineFixture::new(); diff --git a/cmd/devcontainer/src/runtime/engine.rs b/cmd/devcontainer/src/runtime/engine.rs index 0ce143238..e8a8fdc1a 100644 --- a/cmd/devcontainer/src/runtime/engine.rs +++ b/cmd/devcontainer/src/runtime/engine.rs @@ -93,23 +93,40 @@ pub(crate) fn requested_compose_program(args: &[String]) -> Option { ) } -pub(crate) fn compose_pull_always_args(args: &[String]) -> Vec { - if requested_compose_program(args) - .as_deref() - .is_some_and(is_standalone_podman_compose) - { - vec!["--pull-always".to_string()] - } else { - vec!["--pull".to_string(), "always".to_string()] +pub(crate) fn pull_always_requested(args: &[String]) -> bool { + let mut requested = false; + let mut index = 0; + while let Some(arg) = args.get(index) { + if arg == "--pull-always" { + if let Some(value) = args + .get(index + 1) + .and_then(|value| cli_boolean_value(value)) + { + requested = value; + index += 2; + } else { + requested = true; + index += 1; + } + continue; + } + if let Some(value) = arg + .strip_prefix("--pull-always=") + .and_then(cli_boolean_value) + { + requested = value; + } + index += 1; } + requested } -fn is_standalone_podman_compose(program: &str) -> bool { - let filename = program.rsplit(['/', '\\']).next().unwrap_or(program); - Path::new(filename) - .file_stem() - .and_then(|name| name.to_str()) - .is_some_and(|name| name.eq_ignore_ascii_case("podman-compose")) +fn cli_boolean_value(value: &str) -> Option { + match value { + "true" | "1" | "yes" | "on" => Some(true), + "false" | "0" | "no" | "off" => Some(false), + _ => None, + } } fn default_compose_subcommand_available(args: &[String]) -> bool { @@ -215,11 +232,55 @@ mod tests { use crate::process_runner::{ProcessLogLevel, ProcessRequest, ProcessResult}; use super::{ - compose_pull_always_args, compose_request, default_compose_subcommand_available, - engine_request, is_build_request, normalize_process_error, run_compose, run_engine, + compose_request, default_compose_subcommand_available, engine_request, is_build_request, + normalize_process_error, pull_always_requested, run_compose, run_engine, run_engine_streaming, stderr_or_stdout, }; + #[test] + fn pull_always_requested_honors_the_cli_boolean_vocabulary_and_last_value() { + for value in ["true", "1", "yes", "on"] { + assert!( + pull_always_requested(&["--pull-always".to_string(), value.to_string()]), + "spaced {value}" + ); + assert!( + pull_always_requested(&[format!("--pull-always={value}")]), + "equals {value}" + ); + } + for value in ["false", "0", "no", "off"] { + assert!( + !pull_always_requested(&["--pull-always".to_string(), value.to_string()]), + "spaced {value}" + ); + assert!( + !pull_always_requested(&[format!("--pull-always={value}")]), + "equals {value}" + ); + } + + assert!(pull_always_requested(&["--pull-always".to_string()])); + assert!(!pull_always_requested(&[])); + assert!(!pull_always_requested(&[ + "--pull-always=yes".to_string(), + "--pull-always".to_string(), + "off".to_string(), + ])); + assert!(pull_always_requested(&[ + "--pull-always".to_string(), + "false".to_string(), + "--pull-always=on".to_string(), + ])); + assert!(!pull_always_requested(&[ + "--pull-always=invalid".to_string() + ])); + assert!(pull_always_requested(&[ + "--pull-always".to_string(), + "invalid".to_string(), + ])); + } + #[test] fn engine_request_applies_terminal_env_and_log_level() { let request = engine_request( @@ -339,28 +400,6 @@ mod tests { assert_eq!(request.program, "/cli/bin/docker"); } - #[test] - fn compose_pull_always_args_match_the_selected_compose_provider() { - assert_eq!( - compose_pull_always_args(&[]), - vec!["--pull".to_string(), "always".to_string()] - ); - assert_eq!( - compose_pull_always_args(&[ - "--docker-compose-path".to_string(), - "/opt/bin/podman-compose".to_string(), - ]), - vec!["--pull-always".to_string()] - ); - assert_eq!( - compose_pull_always_args(&[ - "--docker-compose-path".to_string(), - "C:\\bin\\podman-compose.exe".to_string(), - ]), - vec!["--pull-always".to_string()] - ); - } - #[test] fn compose_request_uses_env_compose_path_before_engine_probe() { let _env = test_env_defaults(&[ diff --git a/cmd/devcontainer/src/runtime/exec.rs b/cmd/devcontainer/src/runtime/exec.rs index 622977c15..23c5977e3 100644 --- a/cmd/devcontainer/src/runtime/exec.rs +++ b/cmd/devcontainer/src/runtime/exec.rs @@ -31,10 +31,21 @@ impl ExecStdio { } } -pub(crate) fn exec_command_and_args(args: &[String]) -> Result, String> { +pub(crate) struct ParsedExecArguments<'a> { + pub(crate) runtime_args: &'a [String], + pub(crate) command_args: Vec, +} + +pub(crate) fn parse_exec_arguments(args: &[String]) -> Result, String> { let mut index = 0; - while index < args.len() { + let (runtime_end, command_start) = loop { + if index >= args.len() { + return Err("exec requires a command to run".to_string()); + } let arg = &args[index]; + if arg == "--" { + break (index, index + 1); + } if matches!( arg.as_str(), "--docker-path" @@ -78,14 +89,22 @@ pub(crate) fn exec_command_and_args(args: &[String]) -> Result, Stri if arg.starts_with("--") { return Err(format!("Unsupported exec option: {arg}")); } - break; - } + break (index, index); + }; - if index >= args.len() { + if command_start >= args.len() { return Err("exec requires a command to run".to_string()); } - Ok(args[index..].to_vec()) + Ok(ParsedExecArguments { + runtime_args: &args[..runtime_end], + command_args: args[command_start..].to_vec(), + }) +} + +#[cfg_attr(not(test), allow(dead_code))] +pub(crate) fn exec_command_and_args(args: &[String]) -> Result, String> { + parse_exec_arguments(args).map(|parsed| parsed.command_args) } fn is_explicit_bool_literal(value: &str) -> bool { @@ -150,7 +169,8 @@ mod tests { use serde_json::json; use super::{ - exec_command_and_args, exec_engine_args, exec_engine_args_with_remote_env, ExecStdio, + exec_command_and_args, exec_engine_args, exec_engine_args_with_remote_env, + parse_exec_arguments, ExecStdio, }; #[test] @@ -177,6 +197,38 @@ mod tests { assert_eq!(error, "Unsupported exec option: --interactive"); } + #[test] + fn exec_command_and_args_rejects_separator_without_command() { + let error = exec_command_and_args(&["--".to_string()]) + .expect_err("separator must be followed by a command"); + + assert_eq!(error, "exec requires a command to run"); + } + + #[test] + fn exec_command_and_args_strips_separator_and_preserves_payload_flags() { + let input = [ + "--workspace-folder".to_string(), + "/workspace".to_string(), + "--".to_string(), + "--payload-command".to_string(), + "--payload-option".to_string(), + ]; + let parsed = parse_exec_arguments(&input).expect("parsed exec arguments"); + + assert_eq!( + parsed.runtime_args, + &["--workspace-folder".to_string(), "/workspace".to_string()] + ); + assert_eq!( + parsed.command_args, + vec![ + "--payload-command".to_string(), + "--payload-option".to_string() + ] + ); + } + #[test] fn exec_engine_args_include_workdir_user_and_remote_env() { let args = exec_engine_args_with_remote_env( diff --git a/cmd/devcontainer/src/runtime/mod.rs b/cmd/devcontainer/src/runtime/mod.rs index 97e839bb0..eab8a8af7 100644 --- a/cmd/devcontainer/src/runtime/mod.rs +++ b/cmd/devcontainer/src/runtime/mod.rs @@ -180,19 +180,19 @@ pub fn run_user_commands(args: &[String]) -> Result { } pub fn run_exec(args: &[String]) -> Result { - common::validate_runtime_env_defaults(args)?; - let command_args = exec::exec_command_and_args(args)?; - let context = context::resolve_existing_container_context(args)?; + let parsed = exec::parse_exec_arguments(args)?; + common::validate_runtime_env_defaults(parsed.runtime_args)?; + let context = context::resolve_existing_container_context(parsed.runtime_args)?; let engine_args = exec::exec_engine_args( - args, + parsed.runtime_args, &context.configuration, &context.remote_workspace_folder, &context.container_id, - command_args, + parsed.command_args, exec::ExecStdio::current(), )?; - engine::run_engine_streaming(args, engine_args) + engine::run_engine_streaming(parsed.runtime_args, engine_args) } #[cfg(test)] diff --git a/cmd/devcontainer/tests/cli_smoke/collections.rs b/cmd/devcontainer/tests/cli_smoke/collections.rs index 99a470925..a59d55a77 100644 --- a/cmd/devcontainer/tests/cli_smoke/collections.rs +++ b/cmd/devcontainer/tests/cli_smoke/collections.rs @@ -11,6 +11,99 @@ use crate::support::test_support::{devcontainer_command, unique_temp_dir}; const DEFAULT_PUBLISHED_TEMPLATE_BASE_IMAGE: &str = "docker.io/library/debian:bookworm-slim"; +#[test] +fn features_test_array_filters_use_the_default_project_folder() { + let harness = RuntimeHarness::new(); + let workspace = harness.root.join("feature-project"); + for feature in ["demo", "other"] { + let src = workspace.join("src").join(feature); + let test = workspace.join("test").join(feature); + fs::create_dir_all(&src).expect("feature src"); + fs::create_dir_all(&test).expect("feature test"); + fs::write( + src.join("devcontainer-feature.json"), + format!( + "{{\n \"id\": \"{feature}\",\n \"name\": \"{feature}\",\n \"version\": \"1.0.0\"\n}}\n" + ), + ) + .expect("manifest"); + fs::write(test.join("test.sh"), "#!/bin/sh\nexit 0\n").expect("test script"); + } + + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let output = harness.run_in_dir( + &[ + "features", + "test", + "--docker-path", + fake_podman.as_str(), + "--features", + "demo", + "other", + ], + &[], + Some(&workspace), + ); + + assert!(output.status.success(), "{output:?}"); + let stdout = String::from_utf8(output.stdout).expect("utf8 stdout"); + assert!(stdout.contains("'demo'"), "{stdout}"); + assert!(stdout.contains("'other'"), "{stdout}"); + + let _ = fs::remove_dir_all(workspace); +} + +#[test] +fn collection_positionals_work_after_options() { + let root = unique_temp_dir("devcontainer-cli-smoke"); + let feature = root.join("feature"); + let package_output = root.join("package-output"); + fs::create_dir_all(&feature).expect("feature root"); + fs::write( + feature.join("devcontainer-feature.json"), + "{\n \"id\": \"demo\",\n \"name\": \"Demo\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("feature manifest"); + + let package = devcontainer_command(None) + .args([ + "features", + "package", + "--output-folder", + package_output.to_string_lossy().as_ref(), + feature.to_string_lossy().as_ref(), + ]) + .output() + .expect("features package should run"); + assert!(package.status.success(), "{package:?}"); + + let template = root.join("template"); + let template_src = template.join("src"); + let workspace = root.join("workspace"); + fs::create_dir_all(&template_src).expect("template src"); + fs::write( + template.join("devcontainer-template.json"), + "{\n \"id\": \"demo\",\n \"name\": \"Demo\"\n}\n", + ) + .expect("template manifest"); + fs::write(template_src.join("README.md"), "# applied\n").expect("template file"); + + let apply = devcontainer_command(None) + .args([ + "templates", + "apply", + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + template.to_string_lossy().as_ref(), + ]) + .output() + .expect("templates apply should run"); + assert!(apply.status.success(), "{apply:?}"); + assert!(workspace.join("README.md").is_file()); + + let _ = fs::remove_dir_all(root); +} + #[test] fn features_test_emits_a_local_report() { let harness = RuntimeHarness::new(); diff --git a/cmd/devcontainer/tests/cli_smoke/help.rs b/cmd/devcontainer/tests/cli_smoke/help.rs index 883f6247d..9de4772f8 100644 --- a/cmd/devcontainer/tests/cli_smoke/help.rs +++ b/cmd/devcontainer/tests/cli_smoke/help.rs @@ -98,6 +98,140 @@ fn up_rejects_unknown_options_before_dispatch() { } } +#[test] +fn leaf_commands_reject_unknown_arguments_before_dispatch() { + for (args, expected) in [ + ( + ["outdated", "unexpected"].as_slice(), + "Unknown argument: unexpected: devcontainer outdated", + ), + ( + ["outdated", "--unrecognized-option"].as_slice(), + "Unknown option: --unrecognized-option: devcontainer outdated", + ), + ( + ["features", "test", "target", "extra"].as_slice(), + "Unknown argument: extra: devcontainer features test", + ), + ( + ["outdated", "--", "unexpected"].as_slice(), + "Unknown argument: unexpected: devcontainer outdated", + ), + ( + ["build", "--", "first", "second"].as_slice(), + "Unknown argument: second: devcontainer build", + ), + ] { + let output = devcontainer_command(None) + .args(args) + .output() + .expect("command should run"); + + assert_eq!(output.status.code(), Some(2), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(stderr.contains(expected), "{stderr}"); + } +} + +#[test] +fn declared_nested_positionals_reach_command_dispatch() { + for args in [ + ["features", "test", "/definitely/missing/project"].as_slice(), + ["features", "package", "/definitely/missing/project"].as_slice(), + ["templates", "apply", "/definitely/missing/template"].as_slice(), + ["features", "generate-docs", "/definitely/missing/features"].as_slice(), + [ + "templates", + "generate-docs", + "/definitely/missing/templates", + ] + .as_slice(), + ] { + let output = devcontainer_command(None) + .args(args) + .output() + .expect("command should run"); + + assert_ne!(output.status.code(), Some(2), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(!stderr.contains("Unknown argument:"), "{stderr}"); + } +} + +#[test] +fn features_test_accepts_hidden_camel_case_project_folder_alias() { + let output = devcontainer_command(None) + .args([ + "features", + "test", + "--projectFolder=/definitely/missing/project", + ]) + .output() + .expect("features test should run"); + + assert_ne!(output.status.code(), Some(2), "{output:?}"); + let stderr = String::from_utf8(output.stderr).expect("utf8 stderr"); + assert!(!stderr.contains("Unknown option:"), "{stderr}"); + + let help = devcontainer_command(None) + .args(["features", "test", "--help"]) + .output() + .expect("features test help should run"); + let stdout = String::from_utf8(help.stdout).expect("utf8 stdout"); + assert!(!stdout.contains("--projectFolder"), "{stdout}"); +} + +#[test] +fn exec_only_validates_options_before_the_command_payload() { + let rejected = devcontainer_command(None) + .args(["exec", "--payload-option", "/bin/true"]) + .output() + .expect("exec command should run"); + + assert_eq!(rejected.status.code(), Some(2), "{rejected:?}"); + let stderr = String::from_utf8(rejected.stderr).expect("utf8 stderr"); + assert!( + stderr.contains("Unknown option: --payload-option: devcontainer exec"), + "{stderr}" + ); + + let accepted = devcontainer_command(None) + .args([ + "exec", + "--docker-path", + "/definitely/missing/container-engine", + "/bin/true", + "--payload-option", + ]) + .output() + .expect("exec command should run"); + + assert_eq!(accepted.status.code(), Some(1), "{accepted:?}"); + let stderr = String::from_utf8(accepted.stderr).expect("utf8 stderr"); + assert!(!stderr.contains("Unknown option:"), "{stderr}"); + + let after_separator = devcontainer_command(None) + .args([ + "exec", + "--docker-path", + "/definitely/missing/container-engine", + "--", + "--payload-command", + "--payload-option", + ]) + .output() + .expect("exec command should run"); + + assert_eq!( + after_separator.status.code(), + Some(1), + "{after_separator:?}" + ); + let stderr = String::from_utf8(after_separator.stderr).expect("utf8 stderr"); + assert!(!stderr.contains("Unknown option:"), "{stderr}"); + assert!(!stderr.contains("Unsupported exec option:"), "{stderr}"); +} + #[test] fn up_help_lists_upstream_options() { let output = devcontainer_command(None) diff --git a/cmd/devcontainer/tests/cli_smoke/lockfile.rs b/cmd/devcontainer/tests/cli_smoke/lockfile.rs index a5eb7df8e..4448a1e2e 100644 --- a/cmd/devcontainer/tests/cli_smoke/lockfile.rs +++ b/cmd/devcontainer/tests/cli_smoke/lockfile.rs @@ -367,12 +367,9 @@ fn upgrade_with_feature_updates_config_and_dry_run_lockfile() { .args([ "upgrade", "--dry-run", - "--workspace-folder", - workspace.to_string_lossy().as_ref(), - "--feature", - "ghcr.io/codspace/versioning/foo", - "--target-version", - "2", + &format!("--workspace-folder={}", workspace.display()), + "-f=ghcr.io/codspace/versioning/foo", + "-v=2", ]) .output() .expect("upgrade should run"); diff --git a/cmd/devcontainer/tests/runtime_container_smoke/basic.rs b/cmd/devcontainer/tests/runtime_container_smoke/basic.rs index d2d7e8dc5..e0486b2b9 100644 --- a/cmd/devcontainer/tests/runtime_container_smoke/basic.rs +++ b/cmd/devcontainer/tests/runtime_container_smoke/basic.rs @@ -66,7 +66,7 @@ fn up_starts_a_container_and_exec_runs_inside_it() { } #[test] -fn up_pull_always_forwards_the_engine_pull_policy() { +fn up_pull_always_refreshes_the_source_without_pulling_the_final_tag() { let harness = RuntimeHarness::new(); let workspace = harness.workspace(); fs::create_dir_all(&workspace).expect("workspace dir"); @@ -87,12 +87,52 @@ fn up_pull_always_forwards_the_engine_pull_policy() { assert!(output.status.success(), "{output:?}"); let invocations = harness.read_invocations(); + assert!(invocations.contains("pull alpine:3.20"), "{invocations}"); assert!( - invocations.contains("run -d --pull always"), + !invocations.contains("run -d --pull always"), "{invocations}" ); } +#[test] +fn up_pull_always_honors_explicit_true_and_false_values() { + for (arguments, should_pull) in [ + (vec!["--pull-always", "true"], true), + (vec!["--pull-always=true"], true), + (vec!["--pull-always", "false"], false), + (vec!["--pull-always=false"], false), + ] { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(&workspace).expect("workspace dir"); + write_devcontainer_config(&workspace, "{\n \"image\": \"alpine:3.20\"\n}\n"); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + let workspace_arg = workspace.to_string_lossy().to_string(); + let mut args = vec![ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace_arg.as_str(), + ]; + args.extend(arguments.iter().copied()); + + let output = harness.run(&args, &[("FAKE_PODMAN_PS_DISABLE_DEFAULT", "1")]); + + assert!(output.status.success(), "{arguments:?}: {output:?}"); + let invocations = harness.read_invocations(); + assert_eq!( + invocations.contains("pull alpine:3.20"), + should_pull, + "{arguments:?}: {invocations}" + ); + assert!( + !invocations.contains("run -d --pull always"), + "{invocations}" + ); + } +} + #[test] fn up_succeeds_with_env_backed_runtime_defaults() { let harness = RuntimeHarness::new(); diff --git a/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs b/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs index 06ce278ef..c756e0b99 100644 --- a/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs +++ b/cmd/devcontainer/tests/runtime_container_smoke/compose_flow.rs @@ -1,6 +1,6 @@ //! Runtime container smoke tests for compose-backed up flows. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fs; use std::path::Path; @@ -68,6 +68,14 @@ fn compose_label_lookup_args(project_name: &str, service: &str, include_stopped: format!("ps -q{all_arg} --filter label=com.docker.compose.project={project_name} --filter label=com.docker.compose.service={service}") } +fn pulled_images(invocations: &str) -> HashSet<&str> { + invocations + .lines() + .filter_map(|line| line.strip_prefix("pull ")) + .filter_map(|arguments| arguments.split_whitespace().last()) + .collect() +} + #[test] fn up_starts_compose_services_and_exec_uses_compose_container_lookup() { let harness = RuntimeHarness::new(); @@ -138,7 +146,7 @@ fn up_starts_compose_services_and_exec_uses_compose_container_lookup() { } #[test] -fn up_pull_always_forwards_the_compose_pull_policy() { +fn up_pull_always_prepulls_with_the_default_podman_compose_wrapper() { let harness = RuntimeHarness::new(); let workspace = harness.workspace(); fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); @@ -168,7 +176,313 @@ fn up_pull_always_forwards_the_compose_pull_policy() { assert!(output.status.success(), "{output:?}"); let invocations = harness.read_invocations(); assert!( - invocations.contains(" up -d --pull always"), + pulled_images(&invocations) == HashSet::from(["alpine:3.20"]), + "{invocations}" + ); + let up = invocations + .lines() + .find(|line| line.starts_with("compose ") && line.contains(" up ")) + .expect("compose up invocation"); + assert!(up.ends_with(" up -d"), "{invocations}"); + assert!(!up.contains("--pull"), "{invocations}"); +} + +#[test] +fn up_pull_always_reports_underlying_engine_pull_failures_hidden_by_compose() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\"\n}\n", + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[ + ("FAKE_PODMAN_PULL_STDERR", "registry denied the image pull"), + ("FAKE_PODMAN_PULL_EXIT_CODE", "47"), + ], + ); + + assert!(!output.status.success(), "{output:?}"); + assert_eq!( + String::from_utf8(output.stderr) + .expect("utf8 stderr") + .trim(), + "registry denied the image pull" + ); + let invocations = harness.read_invocations(); + assert!(invocations.contains("pull alpine:3.20"), "{invocations}"); + assert!(!invocations.contains(" up -d"), "{invocations}"); +} + +#[test] +fn pull_always_prepull_is_provider_neutral_for_explicit_compose_binaries() { + for compose_name in ["docker-compose", "podman-compose"] { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + let compose_wrapper = harness.root.join(compose_name); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\"\n}\n", + ); + write_executable( + &compose_wrapper, + format!( + "#!/bin/sh\nexec \"{}\" compose \"$@\"\n", + harness.fake_podman.display() + ), + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--docker-compose-path", + compose_wrapper.to_string_lossy().as_ref(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always=true", + ], + &[], + ); + + assert!(output.status.success(), "{compose_name}: {output:?}"); + let invocations = harness.read_invocations(); + assert!( + pulled_images(&invocations) == HashSet::from(["alpine:3.20"]), + "{compose_name}: {invocations}" + ); + assert!( + !invocations + .lines() + .any(|line| line.contains(" up ") && line.contains("--pull")), + "{compose_name}: {invocations}" + ); + } +} + +#[test] +fn pull_always_activates_profiled_run_services_without_wildcard_profile_support() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n worker:\n image: redis:7\n profiles: [debug]\n depends_on:\n db:\n condition: service_started\n db:\n image: postgres:16\n profiles: [data]\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\",\n \"runServices\": [\"worker\"]\n}\n", + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[("FAKE_PODMAN_COMPOSE_REJECT_WILDCARD_PROFILE", "1")], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert!(!invocations.contains("--profile *"), "{invocations}"); + assert_eq!( + pulled_images(&invocations), + HashSet::from(["alpine:3.20", "redis:7", "postgres:16"]), + "{invocations}" + ); + assert!( + invocations + .lines() + .any(|line| line.contains(" up -d worker app")), + "{invocations}" + ); + let compose_files = harness.read_compose_file_log(); + assert!( + compose_files.contains("devcontainer-compose-profile-override"), + "{compose_files}" + ); + assert!( + compose_files.contains(" 'worker':\n profiles: !reset []"), + "{compose_files}" + ); + assert!( + compose_files.contains(" 'db':\n profiles: !reset []"), + "{compose_files}" + ); + assert!( + !compose_files.contains(" 'app':\n profiles: !reset []"), + "{compose_files}" + ); +} + +#[test] +fn pull_always_does_not_pull_compose_generated_images() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n build: .\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\"\n}\n", + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert!( + invocations + .lines() + .any(|line| line.contains(" build --pull app")), + "{invocations}" + ); + assert!(pulled_images(&invocations).is_empty(), "{invocations}"); + assert!( + !invocations + .lines() + .any(|line| line.contains(" up ") && line.contains("--pull")), + "{invocations}" + ); +} + +#[test] +fn pull_always_refreshes_remote_run_services_and_dependencies_without_pulling_builds() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + fs::create_dir_all(workspace.join(".devcontainer")).expect("workspace config dir"); + fs::write( + workspace.join(".devcontainer").join("docker-compose.yml"), + "services:\n app:\n build: .\n depends_on:\n db:\n condition: service_started\n db:\n image: postgres:16\n platform: linux/arm64\n worker:\n image: redis:7\n depends_on:\n cache:\n condition: service_started\n cache:\n image: memcached:1.6\n local-tool:\n build: ./tool\n image: example/local-tool:test\n unrelated:\n image: busybox:1.36\n", + ) + .expect("compose"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\",\n \"runServices\": [\"worker\", \"local-tool\"]\n}\n", + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert_eq!( + pulled_images(&invocations), + HashSet::from(["redis:7", "memcached:1.6", "postgres:16"]), + "{invocations}" + ); + assert!( + invocations.contains("pull --platform linux/arm64 postgres:16"), + "{invocations}" + ); + assert!(invocations.contains(" up -d worker local-tool app")); +} + +#[test] +fn pull_always_refreshes_feature_source_and_image_sidecars_without_repulling_the_result() { + let harness = RuntimeHarness::new(); + let workspace = harness.workspace(); + let config_dir = workspace.join(".devcontainer"); + let feature_dir = config_dir.join("local-feature"); + fs::create_dir_all(&feature_dir).expect("feature dir"); + fs::write( + config_dir.join("docker-compose.yml"), + "services:\n app:\n image: alpine:3.20\n depends_on:\n db:\n condition: service_started\n db:\n image: postgres:16\n worker:\n image: redis:7\n local-tool:\n build: ./tool\n image: example/local-tool:test\n", + ) + .expect("compose"); + fs::write( + feature_dir.join("devcontainer-feature.json"), + "{\n \"id\": \"local-feature\",\n \"name\": \"Local Feature\",\n \"version\": \"1.0.0\"\n}\n", + ) + .expect("feature manifest"); + fs::write(feature_dir.join("install.sh"), "#!/bin/sh\nexit 0\n").expect("feature install"); + write_devcontainer_config( + &workspace, + "{\n \"dockerComposeFile\": \"docker-compose.yml\",\n \"service\": \"app\",\n \"runServices\": [\"worker\", \"local-tool\"],\n \"features\": {\n \"./local-feature\": {}\n }\n}\n", + ); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "up", + "--docker-path", + fake_podman.as_str(), + "--workspace-folder", + workspace.to_string_lossy().as_ref(), + "--pull-always", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + let invocations = harness.read_invocations(); + assert_eq!( + pulled_images(&invocations), + HashSet::from(["alpine:3.20", "redis:7", "postgres:16"]), + "{invocations}" + ); + let feature_build = invocations + .lines() + .find(|line| line.starts_with("build --tag alpine:3.20")) + .expect("feature build invocation"); + assert!(!feature_build.contains("--pull"), "{invocations}"); + assert!( + invocations.find("pull ").expect("pull position") + < invocations.find("build --tag").expect("build position"), "{invocations}" ); } @@ -448,7 +762,13 @@ fn up_uses_env_backed_engine_and_compose_paths_for_compose_workspaces() { let invocations = harness.read_invocations(); assert!(invocations.contains("compose --project-name workspace_devcontainer -f ")); assert!( - invocations.contains(" up -d --pull-always"), + pulled_images(&invocations) == HashSet::from(["alpine:3.20"]), + "{invocations}" + ); + assert!( + !invocations + .lines() + .any(|line| line.contains(" up ") && line.contains("--pull")), "{invocations}" ); } diff --git a/cmd/devcontainer/tests/runtime_exec_smoke.rs b/cmd/devcontainer/tests/runtime_exec_smoke.rs index 440762df2..8dc090506 100644 --- a/cmd/devcontainer/tests/runtime_exec_smoke.rs +++ b/cmd/devcontainer/tests/runtime_exec_smoke.rs @@ -7,6 +7,40 @@ use std::fs; use support::runtime_harness::{write_devcontainer_config, RuntimeHarness}; +#[test] +fn exec_separator_preserves_payload_options() { + let harness = RuntimeHarness::new(); + let fake_podman = harness.fake_podman.to_string_lossy().to_string(); + + let output = harness.run( + &[ + "exec", + "--docker-path", + fake_podman.as_str(), + "--container-id", + "fake-container-id", + "--", + "/bin/echo", + "--workspace-mount-consistency", + "payload-choice", + ], + &[], + ); + + assert!(output.status.success(), "{output:?}"); + assert_eq!( + String::from_utf8(output.stdout).expect("utf8 stdout"), + "--workspace-mount-consistency payload-choice\n" + ); + assert!( + harness + .read_exec_argv_log() + .contains("[/bin/echo]\n[--workspace-mount-consistency]\n[payload-choice]"), + "{}", + harness.read_exec_argv_log() + ); +} + #[test] fn interactive_exec_attaches_stdin() { let harness = RuntimeHarness::new(); diff --git a/cmd/devcontainer/tests/support/runtime_harness/fake_engine.rs b/cmd/devcontainer/tests/support/runtime_harness/fake_engine.rs index 3b95fbd55..140ae258f 100644 --- a/cmd/devcontainer/tests/support/runtime_harness/fake_engine.rs +++ b/cmd/devcontainer/tests/support/runtime_harness/fake_engine.rs @@ -21,6 +21,13 @@ case "$COMMAND" in --project-name) shift 2 ;; + --profile) + if [ "${FAKE_PODMAN_COMPOSE_REJECT_WILDCARD_PROFILE:-0}" = "1" ] && [ "${2:-}" = "*" ]; then + echo "podman-compose does not expand wildcard profiles" >&2 + exit 2 + fi + shift 2 + ;; -f) if [ -n "$COMPOSE_FILES" ]; then COMPOSE_FILES="$COMPOSE_FILES @@ -38,6 +45,25 @@ ${2:-}" SUBCOMMAND="${1:-}" shift || true case "$SUBCOMMAND" in + config) + if [ -n "${FAKE_PODMAN_COMPOSE_CONFIG_FILE:-}" ]; then + cat "${FAKE_PODMAN_COMPOSE_CONFIG_FILE}" + exit 0 + fi + if [ -n "$COMPOSE_FILES" ]; then + old_ifs="${IFS- }" + IFS=' +' + for compose_file in $COMPOSE_FILES; do + if [ -f "$compose_file" ]; then + cat "$compose_file" + break + fi + done + IFS="$old_ifs" + fi + exit 0 + ;; build) compose_file_contents="$LOG_DIR/compose-file-contents.log" : > "$compose_file_contents" @@ -57,6 +83,9 @@ ${2:-}" fi exit 0 ;; + pull) + exit 0 + ;; version) if [ "${1:-}" = "--short" ] && [ -n "${FAKE_PODMAN_COMPOSE_VERSION:-}" ]; then printf '%s\n' "${FAKE_PODMAN_COMPOSE_VERSION}" @@ -222,6 +251,15 @@ ${2:-}" push) exit 0 ;; + pull) + if [ -n "${FAKE_PODMAN_PULL_STDERR:-}" ]; then + printf '%s\n' "${FAKE_PODMAN_PULL_STDERR}" >&2 + fi + if [ -n "${FAKE_PODMAN_PULL_EXIT_CODE:-}" ]; then + exit "${FAKE_PODMAN_PULL_EXIT_CODE}" + fi + exit 0 + ;; run) if [ -n "${FAKE_PODMAN_REQUIRE_FILE_BEFORE_RUN:-}" ] && [ ! -f "${FAKE_PODMAN_REQUIRE_FILE_BEFORE_RUN}" ]; then echo "missing required file before run" >&2 diff --git a/docs/upstream/parity-inventory.json b/docs/upstream/parity-inventory.json index 2293fe069..c4eb67dd7 100644 --- a/docs/upstream/parity-inventory.json +++ b/docs/upstream/parity-inventory.json @@ -721,6 +721,7 @@ "evidence": [ "cmd/devcontainer/src/runtime/build.rs", "cmd/devcontainer/src/runtime/compose/args.rs", + "cmd/devcontainer/src/runtime/compose/mod.rs", "cmd/devcontainer/src/runtime/container/uid_update.rs", "cmd/devcontainer/src/runtime/dockerfile.rs" ] diff --git a/scripts/standalone/real-engine-smoke.sh b/scripts/standalone/real-engine-smoke.sh index 4b152d4db..1748f2569 100755 --- a/scripts/standalone/real-engine-smoke.sh +++ b/scripts/standalone/real-engine-smoke.sh @@ -115,7 +115,7 @@ cat >"$image_workspace/.devcontainer/devcontainer.json" <<'EOF' } EOF -run_devcontainer up --workspace-folder "$image_workspace" >"$tmp_dir/image-up.json" +run_devcontainer up --pull-always --workspace-folder "$image_workspace" >"$tmp_dir/image-up.json" assert_file_contains "$tmp_dir/image-up.json" '"outcome":"success"' image_container_id="$(container_id_from_json "$tmp_dir/image-up.json")" if [[ -z "$image_container_id" ]]; then @@ -162,7 +162,7 @@ cat >"$compose_workspace/.devcontainer/devcontainer.json" <<'EOF' } EOF -run_devcontainer up --workspace-folder "$compose_workspace" >"$tmp_dir/compose-up.json" +run_devcontainer up --pull-always --workspace-folder "$compose_workspace" >"$tmp_dir/compose-up.json" assert_file_contains "$tmp_dir/compose-up.json" '"outcome":"success"' compose_container_id="$(container_id_from_json "$tmp_dir/compose-up.json")" if [[ -z "$compose_container_id" ]]; then