Skip to content

Validate local ClickHouse versions during CLI parsing - #542

Merged
sdairs merged 2 commits into
stack/447-live-cli-postgres-clickpipefrom
stack/463-parse-local-versions
Aug 27, 2026
Merged

Validate local ClickHouse versions during CLI parsing#542
sdairs merged 2 commits into
stack/447-live-cli-postgres-clickpipefrom
stack/463-parse-local-versions

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • parse local install, use, and server ClickHouse version operands into command-specific clap types
  • reject malformed and unsupported version shapes with usage exit 2 before dispatch while preserving Postgres install selectors
  • cover exact parsed variants, actionable errors, telemetry classification, whitespace-normalized selectors, and filesystem side effects

Design notes

  • local remove <version> intentionally remains an opaque string because removal matches exact installed directory names rather than resolving version syntax
  • UseVersionArg and ServerVersionArg intentionally remain separate so each command owns its accepted inputs and diagnostics

Tests

  • cargo test -p clickhousectl
  • cargo test -p clickhousectl local::cli::tests
  • cargo test -p clickhousectl --test local_version_error_test
  • cargo test -p clickhousectl --test telemetry_test invalid_local_versions_report_invalid_value_without_dispatch_side_effects
  • cargo check -p clickhousectl --no-default-features
  • cargo fmt --all --check
  • cargo clippy -p clickhousectl --all-targets -- -D warnings

Closes #463

@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #485 (old PR for #463). This PR is a strict improvement over #485 (per-command types InstallVersionArg/UseVersionArg/ServerVersionArg satisfy acceptance criterion 4 "command-specific semantics explicit rather than assumed identical"; actionable postgres-selector errors; install help now documents postgres@18; no-side-effect assertions; no unrelated test edits). No sdairs review comments existed on #485. Three nits:

  1. Stronger valid-forms assertions. Validate local ClickHouse version operands during CLI parsing #485's clap tests assert exact VersionSpec variants; this PR's valid-forms test only asserts version.to_string() == input round-trips, which is strictly weaker. Assert the parsed VersionSpec variant directly.

  2. Document or validate local remove <version>. It remains an unvalidated String (both PRs). Defensible (remove matches installed dirs), but state that decision somewhere (code comment or PR description).

  3. Consider collapsing UseVersionArg/ServerVersionArg. They are structurally identical and differ only in one error string. Either unify with a parameterized message or note why they are intentionally separate.

Cosmetic: leading-whitespace postgres@ input falls through to the generic numeric-parts error (prefix check precedes the parser's trim).

@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch 2 times, most recently from 5cebc44 to 9e8c38c Compare August 26, 2026 19:01
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch from 9e8c38c to 31cc998 Compare August 26, 2026 19:03
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch from 31cc998 to 1eaaa40 Compare August 26, 2026 19:07
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch 2 times, most recently from 01d7e4f to 1628f33 Compare August 26, 2026 19:12
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch from 1628f33 to 0180a57 Compare August 26, 2026 19:20
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch 2 times, most recently from a10467e to ab7f46b Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch from ab7f46b to 5d6e3d0 Compare August 26, 2026 19:31
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch 3 times, most recently from b6b077e to 561b611 Compare August 26, 2026 20:10
@sdairs
sdairs force-pushed the stack/463-parse-local-versions branch from ffd020c to e569e55 Compare August 27, 2026 10:51
@sdairs
sdairs merged commit 4211ece into main Aug 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate local ClickHouse version operands during CLI parsing

2 participants