Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 154 additions & 19 deletions crates/clickhousectl/src/local/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,8 +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.
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 <name>`.
Expand All @@ -423,8 +423,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<String>,

/// Compatibility form for the server name; prefer positional NAME
#[arg(long = "name", value_name = "NAME", conflicts_with = "name")]
name_flag: Option<String>,

/// System-wide maintenance only: stop a server from any project. You almost certainly want the default project-scoped stop instead.
#[arg(long)]
Expand Down Expand Up @@ -455,12 +459,16 @@ 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.
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")
#[arg(default_value = "default")]
name: String,
#[arg(value_name = "NAME", conflicts_with = "name_flag")]
name: Option<String>,

/// Compatibility form for the server name; prefer positional NAME
#[arg(long = "name", value_name = "NAME", conflicts_with = "name")]
name_flag: Option<String>,
},

/// Write ClickHouse connection env vars to a .env file
Expand Down Expand Up @@ -660,14 +668,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) {
Expand Down Expand Up @@ -1419,25 +1431,144 @@ 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]
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]
Expand Down Expand Up @@ -1578,20 +1709,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, .. },
Expand Down
5 changes: 4 additions & 1 deletion crates/clickhousectl/src/local/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -786,9 +786,11 @@ async fn run_server_commands(command: ServerCommands, json: bool) -> Result<()>
}
ServerCommands::Stop {
name,
name_flag,
global,
project,
} => {
let name = name.or(name_flag).unwrap_or_else(|| "default".to_string());
if global {
stop_server_global(&name, project.as_deref(), json)
} else {
Expand Down Expand Up @@ -845,7 +847,8 @@ 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 } => {
ServerCommands::Remove { name, name_flag } => {
let name = name.or(name_flag).unwrap_or_else(|| "default".to_string());
server::validate_server_name(&name)?;

// Recover orphaned servers so we correctly detect a running
Expand Down
113 changes: 113 additions & 0 deletions crates/clickhousectl/tests/local_server_name_compatibility_test.rs
Original file line number Diff line number Diff line change
@@ -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());
}
}