From 7ff9f16ff17a996e9582edcf24e2d74d8451e903 Mon Sep 17 00:00:00 2001 From: sdairs Date: Wed, 26 Aug 2026 15:54:02 +0100 Subject: [PATCH 1/2] Add server name compatibility flags --- crates/clickhousectl/src/local/cli.rs | 154 ++++++++++++++++-- crates/clickhousectl/src/local/mod.rs | 132 ++++++++------- .../local_server_name_compatibility_test.rs | 113 +++++++++++++ 3 files changed, 323 insertions(+), 76 deletions(-) create mode 100644 crates/clickhousectl/tests/local_server_name_compatibility_test.rs diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index d7987172..7a5a93be 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -415,6 +415,8 @@ CONTEXT FOR AGENTS: CONTEXT FOR AGENTS: Stops a ClickHouse server. The name defaults to \"default\"; use `clickhousectl local server list` to find other server names. + Pass the name positionally (e.g. `server stop dev`). The compatibility form `--name dev` + remains accepted, but cannot be combined with a positional name. Sends SIGTERM first, then SIGKILL if the process doesn't exit gracefully. The server's data and metadata are preserved so it remains visible in `server list`. Restart with `clickhousectl local server start `. @@ -423,8 +425,12 @@ CONTEXT FOR AGENTS: Related: `clickhousectl local server list` to see servers.")] Stop { /// Name of the server to stop (default: "default") - #[arg(default_value = "default")] - name: String, + #[arg(value_name = "NAME", conflicts_with = "name_flag")] + name: Option, + + /// Compatibility form for the server name; prefer positional NAME + #[arg(long = "name", value_name = "NAME", conflicts_with = "name")] + name_flag: Option, /// System-wide maintenance only: stop a server from any project. You almost certainly want the default project-scoped stop instead. #[arg(long)] @@ -455,12 +461,18 @@ CONTEXT FOR AGENTS: CONTEXT FOR AGENTS: Permanently deletes a server's data directory. The server must be stopped first. This is irreversible — all data for this server instance will be lost. + Pass the name positionally (e.g. `server remove dev`). The compatibility form `--name dev` + remains accepted, but cannot be combined with a positional name. The name defaults to \"default\". Related: `clickhousectl local server stop [name]` to stop first, `clickhousectl local server list` to see servers.")] Remove { /// Name of the server to remove (default: "default") - #[arg(default_value = "default")] - name: String, + #[arg(value_name = "NAME", conflicts_with = "name_flag")] + name: Option, + + /// Compatibility form for the server name; prefer positional NAME + #[arg(long = "name", value_name = "NAME", conflicts_with = "name")] + name_flag: Option, }, /// Write ClickHouse connection env vars to a .env file @@ -660,14 +672,18 @@ mod tests { use crate::version_manager::list::Channel; use clap::Parser; - fn local_command(args: &[&str]) -> LocalCommands { + fn local_args(args: &[&str]) -> LocalArgs { let mut argv = vec!["clickhousectl", "local"]; argv.extend_from_slice(args); let cli = Cli::try_parse_from(argv).unwrap(); let Commands::Local(local) = cli.command else { panic!("expected local command"); }; - local.command + local + } + + fn local_command(args: &[&str]) -> LocalCommands { + local_args(args).command } fn assert_version_rejected(args: &[&str], expected: &str) { @@ -1419,25 +1435,127 @@ mod tests { } #[test] - fn server_stop_name_defaults_to_default() { + fn server_stop_omitted_name_remains_distinguishable() { let LocalCommands::Server { - command: ServerCommands::Stop { name, .. }, + command: ServerCommands::Stop { + name, name_flag, .. + }, } = local_command(&["server", "stop"]) else { panic!("expected server stop"); }; - assert_eq!(name, "default"); + assert_eq!(name, None); + assert_eq!(name_flag, None); } #[test] - fn server_remove_name_defaults_to_default() { + fn server_remove_omitted_name_remains_distinguishable() { let LocalCommands::Server { - command: ServerCommands::Remove { name }, + command: ServerCommands::Remove { name, name_flag }, } = local_command(&["server", "remove"]) else { panic!("expected server remove"); }; - assert_eq!(name, "default"); + assert_eq!(name, None); + assert_eq!(name_flag, None); + } + + #[test] + fn server_stop_name_forms_allow_trailing_options() { + for (command, expected_name, expected_name_flag) in [ + ( + &[ + "server", + "stop", + "analytics", + "--global", + "--project", + "/tmp/project", + "--json", + ][..], + Some("analytics"), + None, + ), + ( + &[ + "server", + "stop", + "--name", + "analytics", + "--global", + "--project", + "/tmp/project", + "--json", + ][..], + None, + Some("analytics"), + ), + ] { + let args = local_args(command); + assert!(args.json); + let LocalCommands::Server { + command: + ServerCommands::Stop { + name, + name_flag, + global, + project, + }, + } = args.command + else { + panic!("expected server stop"); + }; + assert_eq!(name.as_deref(), expected_name); + assert_eq!(name_flag.as_deref(), expected_name_flag); + assert!(global); + assert_eq!(project.as_deref(), Some("/tmp/project")); + } + } + + #[test] + fn server_remove_name_forms_allow_trailing_options() { + for (command, expected_name, expected_name_flag) in [ + ( + &["server", "remove", "analytics", "--json"][..], + Some("analytics"), + None, + ), + ( + &["server", "remove", "--name", "analytics", "--json"][..], + None, + Some("analytics"), + ), + ] { + let args = local_args(command); + assert!(args.json); + let LocalCommands::Server { + command: ServerCommands::Remove { name, name_flag }, + } = args.command + else { + panic!("expected server remove"); + }; + assert_eq!(name.as_deref(), expected_name); + assert_eq!(name_flag.as_deref(), expected_name_flag); + } + } + + #[test] + fn server_teardown_name_forms_conflict() { + for command in ["stop", "remove"] { + let error = Cli::try_parse_from([ + "clickhousectl", + "local", + "server", + command, + "positional", + "--name", + "flagged", + ]) + .err() + .expect("name forms should conflict"); + assert_eq!(error.kind(), clap::error::ErrorKind::ArgumentConflict); + assert!(error.to_string().contains("cannot be used with"), "{error}"); + } } #[test] @@ -1578,20 +1696,24 @@ mod tests { #[test] fn teardown_commands_preserve_explicit_names() { let LocalCommands::Server { - command: ServerCommands::Stop { name, .. }, + command: ServerCommands::Stop { + name, name_flag, .. + }, } = local_command(&["server", "stop", "analytics"]) else { panic!("expected server stop"); }; - assert_eq!(name, "analytics"); + assert_eq!(name.as_deref(), Some("analytics")); + assert_eq!(name_flag, None); let LocalCommands::Server { - command: ServerCommands::Remove { name }, + command: ServerCommands::Remove { name, name_flag }, } = local_command(&["server", "remove", "analytics"]) else { panic!("expected server remove"); }; - assert_eq!(name, "analytics"); + assert_eq!(name.as_deref(), Some("analytics")); + assert_eq!(name_flag, None); let LocalCommands::Postgres { command: PostgresCommands::Stop { name, .. }, diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 692a9d58..920d305c 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -786,51 +786,10 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> } ServerCommands::Stop { name, + name_flag, global, project, - } => { - if global { - stop_server_global(&name, project.as_deref(), json) - } else { - server::validate_server_name(&name)?; - - // Recover orphaned servers so we can stop processes - // that lost their metadata files. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - - match classify_stop( - server::is_server_running_locked(&name, &metadata_lock)?, - server::server_data_dir(&name).exists(), - ) { - StopOutcome::Stop => { - if !json { - println!("Stopping server '{}'...", name); - } - server::kill_server_locked(&name, &metadata_lock)?; - let out = output::ServerStopOutput { - name, - already_stopped: false, - }; - output::print_output(&out, json); - Ok(()) - } - StopOutcome::AlreadyStopped => { - // Server exists on disk but isn't running. `stop` is - // idempotent: this is the desired end state, so succeed - // instead of erroring. - let out = output::ServerStopOutput { - name, - already_stopped: true, - }; - output::print_output(&out, json); - Ok(()) - } - // No such server in this project — surface the typo. - StopOutcome::NotFound => Err(Error::ServerNotFound(name)), - } - } - } + } => stop_server(name.or(name_flag), global, project, json), ServerCommands::StopAll { global } => { if global { stop_all_servers_global(json) @@ -845,30 +804,83 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> password, database, } => dotenv_server(name.as_deref(), local, user, password, database, json), - ServerCommands::Remove { name } => { - server::validate_server_name(&name)?; + ServerCommands::Remove { name, name_flag } => remove_server(name.or(name_flag), json), + } +} - // Recover orphaned servers so we correctly detect a running - // process even when its metadata file is missing. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; +fn stop_server( + name: Option, + global: bool, + project: Option, + json: bool, +) -> Result<()> { + let name = name.unwrap_or_else(|| "default".to_string()); + if global { + return stop_server_global(&name, project.as_deref(), json); + } - if server::is_server_running_locked(&name, &metadata_lock)? { - return Err(Error::ServerRunningCannotRemove(name)); - } - let data_dir = server::server_data_dir(&name); - if !data_dir.exists() { - return Err(Error::ServerNotFound(name)); + server::validate_server_name(&name)?; + + // Recover orphaned servers so we can stop processes + // that lost their metadata files. + let metadata_lock = server::lock_metadata()?; + server::recover_current_project_servers_locked(&metadata_lock)?; + + match classify_stop( + server::is_server_running_locked(&name, &metadata_lock)?, + server::server_data_dir(&name).exists(), + ) { + StopOutcome::Stop => { + if !json { + println!("Stopping server '{}'...", name); } - // Remove the whole server directory (parent of data/) - let server_dir = data_dir.parent().unwrap(); - std::fs::remove_dir_all(server_dir)?; - server::try_remove_server_info_locked(&name, &metadata_lock)?; - let out = output::ServerRemoveOutput { name }; + server::kill_server_locked(&name, &metadata_lock)?; + let out = output::ServerStopOutput { + name, + already_stopped: false, + }; output::print_output(&out, json); Ok(()) } + StopOutcome::AlreadyStopped => { + // Server exists on disk but isn't running. `stop` is + // idempotent: this is the desired end state, so succeed + // instead of erroring. + let out = output::ServerStopOutput { + name, + already_stopped: true, + }; + output::print_output(&out, json); + Ok(()) + } + // No such server in this project — surface the typo. + StopOutcome::NotFound => Err(Error::ServerNotFound(name)), + } +} + +fn remove_server(name: Option, json: bool) -> Result<()> { + let name = name.unwrap_or_else(|| "default".to_string()); + server::validate_server_name(&name)?; + + // Recover orphaned servers so we correctly detect a running + // process even when its metadata file is missing. + let metadata_lock = server::lock_metadata()?; + server::recover_current_project_servers_locked(&metadata_lock)?; + + if server::is_server_running_locked(&name, &metadata_lock)? { + return Err(Error::ServerRunningCannotRemove(name)); + } + let data_dir = server::server_data_dir(&name); + if !data_dir.exists() { + return Err(Error::ServerNotFound(name)); } + // Remove the whole server directory (parent of data/) + let server_dir = data_dir.parent().unwrap(); + std::fs::remove_dir_all(server_dir)?; + server::try_remove_server_info_locked(&name, &metadata_lock)?; + let out = output::ServerRemoveOutput { name }; + output::print_output(&out, json); + Ok(()) } /// What a project-scoped `server stop ` should do, given whether the diff --git a/crates/clickhousectl/tests/local_server_name_compatibility_test.rs b/crates/clickhousectl/tests/local_server_name_compatibility_test.rs new file mode 100644 index 00000000..fa670a3d --- /dev/null +++ b/crates/clickhousectl/tests/local_server_name_compatibility_test.rs @@ -0,0 +1,113 @@ +//! Subprocess coverage for ClickHouse server name compatibility (issue #474). + +use serde_json::Value; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn run(project: &Path, home: &Path, args: &[&str]) -> Output { + Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .current_dir(project) + .args(args) + .output() + .expect("run clickhousectl") +} + +fn create_stopped_server(project: &Path, name: &str) -> PathBuf { + let directory = project.join(".clickhouse/servers").join(name); + std::fs::create_dir_all(directory.join("data")).expect("create stopped server data"); + directory +} + +fn assert_success(output: &Output) { + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn assert_stop_dispatch(name_args: &[&str], expected: &str) { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + create_stopped_server(project.path(), expected); + let decoy = create_stopped_server(project.path(), "decoy"); + let mut args = vec!["local", "server", "stop"]; + args.extend_from_slice(name_args); + args.push("--json"); + + let output = run(project.path(), home.path(), &args); + + assert_success(&output); + let body: Value = serde_json::from_slice(&output.stdout).expect("parse stop JSON"); + assert_eq!(body["name"], expected); + assert_eq!(body["already_stopped"], true); + assert!(decoy.exists()); +} + +fn assert_remove_dispatch(name_args: &[&str], expected: &str) { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + let selected = create_stopped_server(project.path(), expected); + let decoy = create_stopped_server(project.path(), "decoy"); + let mut args = vec!["local", "server", "remove"]; + args.extend_from_slice(name_args); + args.push("--json"); + + let output = run(project.path(), home.path(), &args); + + assert_success(&output); + let body: Value = serde_json::from_slice(&output.stdout).expect("parse remove JSON"); + assert_eq!(body["name"], expected); + assert!(!selected.exists()); + assert!(decoy.exists()); +} + +#[test] +fn stop_dispatches_positional_and_compatibility_names_exactly() { + assert_stop_dispatch(&["positional-stop"], "positional-stop"); + assert_stop_dispatch(&["--name", "flag-stop"], "flag-stop"); +} + +#[test] +fn remove_dispatches_positional_and_compatibility_names_exactly() { + assert_remove_dispatch(&["positional-remove"], "positional-remove"); + assert_remove_dispatch(&["--name", "flag-remove"], "flag-remove"); +} + +#[test] +fn omitted_names_keep_the_current_default_runtime_behavior() { + assert_stop_dispatch(&[], "default"); + assert_remove_dispatch(&[], "default"); +} + +#[test] +fn conflicting_name_forms_fail_before_dispatch() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + + for command in ["stop", "remove"] { + let output = run( + project.path(), + home.path(), + &[ + "local", + "server", + command, + "positional", + "--name", + "flagged", + ], + ); + + assert_eq!(output.status.code(), Some(2)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("cannot be used with"), "stderr: {stderr}"); + assert!(!project.path().join(".clickhouse").exists()); + } +} From 23f23b9f558b2cbb7a838f3f9e598dd71f6c4569 Mon Sep 17 00:00:00 2001 From: sdairs Date: Thu, 27 Aug 2026 08:10:46 +0100 Subject: [PATCH 2/2] Address server name compatibility review --- crates/clickhousectl/src/local/cli.rs | 27 ++++-- crates/clickhousectl/src/local/mod.rs | 133 ++++++++++++-------------- 2 files changed, 82 insertions(+), 78 deletions(-) diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 7a5a93be..62fc2892 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -413,10 +413,8 @@ CONTEXT FOR AGENTS: /// Stop a running server by name #[command(after_help = "\ CONTEXT FOR AGENTS: - Stops a ClickHouse server. The name defaults to \"default\"; use `clickhousectl local server list` - to find other server names. - Pass the name positionally (e.g. `server stop dev`). The compatibility form `--name dev` - remains accepted, but cannot be combined with a positional name. + Stops a ClickHouse server. The name defaults to \"default\"; pass it positionally to select + another server (e.g., `server stop dev`). Use `clickhousectl local server list` to find names. Sends SIGTERM first, then SIGKILL if the process doesn't exit gracefully. The server's data and metadata are preserved so it remains visible in `server list`. Restart with `clickhousectl local server start `. @@ -461,9 +459,7 @@ CONTEXT FOR AGENTS: CONTEXT FOR AGENTS: Permanently deletes a server's data directory. The server must be stopped first. This is irreversible — all data for this server instance will be lost. - Pass the name positionally (e.g. `server remove dev`). The compatibility form `--name dev` - remains accepted, but cannot be combined with a positional name. - The name defaults to \"default\". + The name defaults to \"default\"; pass it positionally to select another server. Related: `clickhousectl local server stop [name]` to stop first, `clickhousectl local server list` to see servers.")] Remove { /// Name of the server to remove (default: "default") @@ -1558,6 +1554,23 @@ mod tests { } } + #[test] + fn server_teardown_agent_help_only_advertises_positional_names() { + for command in ["stop", "remove"] { + let help = Cli::try_parse_from(["clickhousectl", "local", "server", command, "--help"]) + .err() + .expect("help should exit through clap") + .to_string(); + let context = help + .split_once("CONTEXT FOR AGENTS:") + .expect("agent context") + .1; + + assert!(context.contains("positionally"), "{help}"); + assert!(!context.contains("--name"), "{help}"); + } + } + #[test] fn server_stop_all_help_describes_engine_scope() { let help = Cli::try_parse_from(["clickhousectl", "local", "server", "stop-all", "--help"]) diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index 920d305c..40cef850 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -789,7 +789,50 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> name_flag, global, project, - } => stop_server(name.or(name_flag), global, project, json), + } => { + let name = name.or(name_flag).unwrap_or_else(|| "default".to_string()); + if global { + stop_server_global(&name, project.as_deref(), json) + } else { + server::validate_server_name(&name)?; + + // Recover orphaned servers so we can stop processes + // that lost their metadata files. + let metadata_lock = server::lock_metadata()?; + server::recover_current_project_servers_locked(&metadata_lock)?; + + match classify_stop( + server::is_server_running_locked(&name, &metadata_lock)?, + server::server_data_dir(&name).exists(), + ) { + StopOutcome::Stop => { + if !json { + println!("Stopping server '{}'...", name); + } + server::kill_server_locked(&name, &metadata_lock)?; + let out = output::ServerStopOutput { + name, + already_stopped: false, + }; + output::print_output(&out, json); + Ok(()) + } + StopOutcome::AlreadyStopped => { + // Server exists on disk but isn't running. `stop` is + // idempotent: this is the desired end state, so succeed + // instead of erroring. + let out = output::ServerStopOutput { + name, + already_stopped: true, + }; + output::print_output(&out, json); + Ok(()) + } + // No such server in this project — surface the typo. + StopOutcome::NotFound => Err(Error::ServerNotFound(name)), + } + } + } ServerCommands::StopAll { global } => { if global { stop_all_servers_global(json) @@ -804,83 +847,31 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> password, database, } => dotenv_server(name.as_deref(), local, user, password, database, json), - ServerCommands::Remove { name, name_flag } => remove_server(name.or(name_flag), json), - } -} + ServerCommands::Remove { name, name_flag } => { + let name = name.or(name_flag).unwrap_or_else(|| "default".to_string()); + server::validate_server_name(&name)?; -fn stop_server( - name: Option, - global: bool, - project: Option, - json: bool, -) -> Result<()> { - let name = name.unwrap_or_else(|| "default".to_string()); - if global { - return stop_server_global(&name, project.as_deref(), json); - } - - server::validate_server_name(&name)?; + // Recover orphaned servers so we correctly detect a running + // process even when its metadata file is missing. + let metadata_lock = server::lock_metadata()?; + server::recover_current_project_servers_locked(&metadata_lock)?; - // Recover orphaned servers so we can stop processes - // that lost their metadata files. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - - match classify_stop( - server::is_server_running_locked(&name, &metadata_lock)?, - server::server_data_dir(&name).exists(), - ) { - StopOutcome::Stop => { - if !json { - println!("Stopping server '{}'...", name); + if server::is_server_running_locked(&name, &metadata_lock)? { + return Err(Error::ServerRunningCannotRemove(name)); } - server::kill_server_locked(&name, &metadata_lock)?; - let out = output::ServerStopOutput { - name, - already_stopped: false, - }; - output::print_output(&out, json); - Ok(()) - } - StopOutcome::AlreadyStopped => { - // Server exists on disk but isn't running. `stop` is - // idempotent: this is the desired end state, so succeed - // instead of erroring. - let out = output::ServerStopOutput { - name, - already_stopped: true, - }; + let data_dir = server::server_data_dir(&name); + if !data_dir.exists() { + return Err(Error::ServerNotFound(name)); + } + // Remove the whole server directory (parent of data/) + let server_dir = data_dir.parent().unwrap(); + std::fs::remove_dir_all(server_dir)?; + server::try_remove_server_info_locked(&name, &metadata_lock)?; + let out = output::ServerRemoveOutput { name }; output::print_output(&out, json); Ok(()) } - // No such server in this project — surface the typo. - StopOutcome::NotFound => Err(Error::ServerNotFound(name)), - } -} - -fn remove_server(name: Option, json: bool) -> Result<()> { - let name = name.unwrap_or_else(|| "default".to_string()); - server::validate_server_name(&name)?; - - // Recover orphaned servers so we correctly detect a running - // process even when its metadata file is missing. - let metadata_lock = server::lock_metadata()?; - server::recover_current_project_servers_locked(&metadata_lock)?; - - if server::is_server_running_locked(&name, &metadata_lock)? { - return Err(Error::ServerRunningCannotRemove(name)); - } - let data_dir = server::server_data_dir(&name); - if !data_dir.exists() { - return Err(Error::ServerNotFound(name)); } - // Remove the whole server directory (parent of data/) - let server_dir = data_dir.parent().unwrap(); - std::fs::remove_dir_all(server_dir)?; - server::try_remove_server_info_locked(&name, &metadata_lock)?; - let out = output::ServerRemoveOutput { name }; - output::print_output(&out, json); - Ok(()) } /// What a project-scoped `server stop ` should do, given whether the