diff --git a/README.md b/README.md index 2d0d3e4..c31697e 100644 --- a/README.md +++ b/README.md @@ -162,12 +162,13 @@ A bare `clickhousectl local server start` bootstraps from zero: if no version is ```bash # Start a server (runs in background by default) clickhousectl local server start # Named "default" (installs latest if nothing is set up yet) -clickhousectl local server start --name dev # Named "dev" +clickhousectl local server start dev # Named "dev" clickhousectl local server start --version latest # Use a specific version (installs if needed, doesn't change default) clickhousectl local server start --foreground # Run in foreground (-F / --fg) clickhousectl local server start --no-wait # Return after spawning without waiting for readiness clickhousectl local server start --http-port 8124 --tcp-port 9001 # Explicit ports clickhousectl local server start --config analytics # Apply a custom config (see "Custom config files" below) +clickhousectl local server start dev -- --logger.level=trace # Pass clickhouse-server arguments after -- # List custom config files available to --config clickhousectl local server configs @@ -199,7 +200,9 @@ clickhousectl local server dotenv --user default --password secret --database my Stopping a server preserves its data and identity metadata, so it remains visible in `server list` with a `stopped` status. Version and ports are shown only while running because they are resolved again on each start. Starting the same name resumes the existing data directory. -**Server naming:** Without `--name`, the first server is called "default". If "default" is already running, a random name is generated (e.g. "bold-crane"). Use `--name` for stable identities you can start/stop repeatedly. +**Server naming:** Without a name, the first server is called "default". If "default" is already running, a random name is generated (e.g. "bold-crane"). Pass a name positionally for stable identities you can start/stop repeatedly. The existing `--name ` form remains accepted for compatibility, but cannot be combined with a positional name. + +**ClickHouse arguments:** Additional `clickhouse-server` arguments must follow `--`. This boundary keeps clickhousectl options such as `--version` unambiguous after the optional server name. **Ports:** Defaults are HTTP 8123 and TCP 9000. If these are already in use, free ports are automatically assigned and shown in the output. Use `--http-port` and `--tcp-port` to set explicit ports. diff --git a/crates/clickhousectl/src/local/cli.rs b/crates/clickhousectl/src/local/cli.rs index 860da6f..6f1bf71 100644 --- a/crates/clickhousectl/src/local/cli.rs +++ b/crates/clickhousectl/src/local/cli.rs @@ -136,7 +136,7 @@ CONTEXT FOR AGENTS: here manage ClickHouse. Each server has its own data directory. Data is stored in .clickhouse/servers//data/ and persists between restarts. - Typical: `clickhousectl local server start` (starts \"default\"), `clickhousectl local server start --name test`. + Typical: `clickhousectl local server start` (starts \"default\"), `clickhousectl local server start test`. Related: `clickhousectl local client` to connect to a running server.")] Server { #[command(subcommand)] @@ -165,9 +165,10 @@ pub enum ServerCommands { CONTEXT FOR AGENTS: Starts a named clickhouse-server instance with its own data directory. Data is stored in .clickhouse/servers//data/ and persists between restarts. - Without --name, the first server is called \"default\"; if \"default\" is already running, + Without a name, the first server is called \"default\"; if \"default\" is already running, a random name is generated (e.g., \"bold-crane\"). - Use --name to give a server a stable identity (e.g., --name dev, --name test). + Pass the name positionally to give a server a stable identity (e.g., `server start dev`). + The older `--name dev` form remains accepted, but cannot be combined with a positional name. Use --version (-v) to run a specific ClickHouse version without changing the default. Accepts same specs as install/use: \"latest\" (recommended), stable, lts, 25.12, etc. Installs if needed. With no --version and no default set, a bare start bootstraps by installing \"latest\" (without @@ -176,19 +177,24 @@ CONTEXT FOR AGENTS: Use --http-port and --tcp-port to set explicit ports. Runs in background by default. Use --foreground (-F / --fg) to run in foreground. Background starts wait for HTTP health and TCP connections. Use --no-wait to return after spawning. - If --name is given and that server is already running, the command will error. + If a name is given and that server is already running, the command will error. Shows count of already-running servers before starting. Use --config to apply a custom ClickHouse config file from ~/.clickhouse/configs/ (see `clickhousectl local server configs`). The file is merged as an overlay on top of ClickHouse's built-in defaults (via config.d), so it can contain just the settings you want to change (e.g. ). The data directory and ports stay managed regardless of the file's contents (they are forced as command-line overrides). + Additional clickhouse-server arguments must follow `--`. Related: `clickhousectl local server list` to see servers, `clickhousectl local server stop [name]` to stop one.")] Start { /// Server name (default: \"default\", or random if default is already running) - #[arg(long)] + #[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, + /// ClickHouse version to use (e.g. "latest" (recommended), stable, lts, 25.12). Installs if needed. Does not change the default version. #[arg(long, short = 'v')] version: Option, @@ -213,8 +219,8 @@ CONTEXT FOR AGENTS: #[arg(long = "config", alias = "config-file", value_name = "NAME")] config_file: Option, - /// Arguments to pass to clickhouse-server - #[arg(trailing_var_arg = true, allow_hyphen_values = true)] + /// Arguments to pass to clickhouse-server after `--` + #[arg(last = true, allow_hyphen_values = true, value_name = "CLICKHOUSE_ARG")] args: Vec, }, @@ -250,7 +256,7 @@ CONTEXT FOR AGENTS: to find other server 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 --name `. + Restart with `clickhousectl local server start `. Idempotent: a server that exists but is already stopped exits 0 (no error). An unknown server name still errors so typos are caught. Related: `clickhousectl local server list` to see servers.")] @@ -539,6 +545,100 @@ mod tests { assert!(args.is_empty()); } + #[test] + fn parses_server_start_positional_name_before_clickhousectl_options() { + let LocalCommands::Server { + command: + ServerCommands::Start { + name, + name_flag, + version, + args, + .. + }, + } = local_command(&["server", "start", "existing", "--version", "25.12.9.61"]) + else { + panic!("expected server start"); + }; + assert_eq!(name.as_deref(), Some("existing")); + assert_eq!(name_flag, None); + assert_eq!(version.as_deref(), Some("25.12.9.61")); + assert!(args.is_empty()); + } + + #[test] + fn parses_server_start_name_flag_for_compatibility() { + let LocalCommands::Server { + command: ServerCommands::Start { + name, name_flag, .. + }, + } = local_command(&["server", "start", "--name", "existing"]) + else { + panic!("expected server start"); + }; + assert_eq!(name, None); + assert_eq!(name_flag.as_deref(), Some("existing")); + } + + #[test] + fn server_start_name_forms_conflict() { + let error = Cli::try_parse_from([ + "clickhousectl", + "local", + "server", + "start", + "existing", + "--name", + "other", + ]) + .err() + .expect("name forms should conflict"); + assert_eq!(error.kind(), clap::error::ErrorKind::ArgumentConflict); + } + + #[test] + fn server_start_passthrough_requires_boundary() { + let LocalCommands::Server { + command: + ServerCommands::Start { + name, + version, + args, + .. + }, + } = local_command(&[ + "server", + "start", + "existing", + "--version", + "25.12.9.61", + "--", + "--logger.level=trace", + "--max_server_memory_usage=1000000", + ]) + else { + panic!("expected server start"); + }; + assert_eq!(name.as_deref(), Some("existing")); + assert_eq!(version.as_deref(), Some("25.12.9.61")); + assert_eq!( + args, + ["--logger.level=trace", "--max_server_memory_usage=1000000"] + ); + + let error = Cli::try_parse_from([ + "clickhousectl", + "local", + "server", + "start", + "existing", + "--logger.level=trace", + ]) + .err() + .expect("passthrough without -- should fail"); + assert_eq!(error.kind(), clap::error::ErrorKind::UnknownArgument); + } + #[test] fn parses_server_start_no_wait() { let LocalCommands::Server { diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index e4b93c4..442d75c 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -682,6 +682,7 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> match command { ServerCommands::Start { name, + name_flag, version, http_port, tcp_port, @@ -691,7 +692,7 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()> args, } => { start_server( - name, + name.or(name_flag), version, http_port, tcp_port, diff --git a/crates/clickhousectl/tests/local_server_start_args_test.rs b/crates/clickhousectl/tests/local_server_start_args_test.rs new file mode 100644 index 0000000..761eaa4 --- /dev/null +++ b/crates/clickhousectl/tests/local_server_start_args_test.rs @@ -0,0 +1,115 @@ +//! Regression coverage for server start argument parsing (issue #357). + +use serde_json::Value; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::time::{Duration, Instant}; + +const DEFAULT_VERSION: &str = "25.11.1.1"; +const REQUESTED_VERSION: &str = "25.12.9.61"; + +fn clickhousectl_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_clickhousectl")) +} + +fn install_fake_clickhouse(home: &Path, version: &str) { + let binary = home + .join(".clickhouse/versions") + .join(version) + .join("clickhouse"); + std::fs::create_dir_all(binary.parent().unwrap()).expect("create fake version dir"); + std::fs::write( + &binary, + b"#!/bin/sh\nprintf '%s\\n' \"$@\" > \"$FAKE_CLICKHOUSE_ARGS_FILE\"\nexec sleep 30\n", + ) + .expect("write fake ClickHouse"); + let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(binary, permissions).expect("make fake ClickHouse executable"); +} + +fn run_start(project: &Path, home: &Path, args_file: &Path) -> Output { + Command::new(clickhousectl_binary()) + .env("DO_NOT_TRACK", "1") + .env("HOME", home) + .env("FAKE_CLICKHOUSE_ARGS_FILE", args_file) + .current_dir(project) + .args([ + "local", + "--json", + "server", + "start", + "--no-wait", + "existing", + "--version", + REQUESTED_VERSION, + "--", + "--logger.level=trace", + ]) + .output() + .expect("run clickhousectl") +} + +fn read_file_eventually(path: &Path) -> String { + let deadline = Instant::now() + Duration::from_secs(5); + loop { + match std::fs::read_to_string(path) { + Ok(contents) if !contents.is_empty() => return contents, + Ok(_) => {} + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + Err(error) => panic!("read fake ClickHouse arguments: {error}"), + } + assert!(Instant::now() < deadline, "fake ClickHouse did not start"); + std::thread::sleep(Duration::from_millis(10)); + } +} + +struct ProcessGuard(u32); + +impl Drop for ProcessGuard { + fn drop(&mut self) { + unsafe { + libc::kill(self.0 as i32, libc::SIGKILL); + } + } +} + +#[test] +fn positional_name_keeps_following_version_and_passthrough_separate() { + let project = tempfile::tempdir().expect("create project tempdir"); + let home = tempfile::tempdir().expect("create home tempdir"); + install_fake_clickhouse(home.path(), DEFAULT_VERSION); + install_fake_clickhouse(home.path(), REQUESTED_VERSION); + std::fs::write(home.path().join(".clickhouse/default"), DEFAULT_VERSION) + .expect("write default version"); + let args_file = home.path().join("clickhouse-args.txt"); + + let output = run_start(project.path(), home.path(), &args_file); + assert!( + output.status.success(), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let body: Value = serde_json::from_slice(&output.stdout).expect("parse start JSON"); + let _process = ProcessGuard(body["pid"].as_u64().expect("start PID") as u32); + + assert_eq!(body["name"], "existing"); + assert_eq!(body["version"], REQUESTED_VERSION); + assert!( + project + .path() + .join(".clickhouse/servers/existing.json") + .exists() + ); + assert!( + !project + .path() + .join(".clickhouse/servers/default.json") + .exists() + ); + + let child_args = read_file_eventually(&args_file); + assert_eq!(child_args.lines().last(), Some("--logger.level=trace")); + assert!(!child_args.lines().any(|arg| arg == "--version")); +}