Skip to content

Make repeated exact installs idempotent - #349

Open
sdairs wants to merge 1 commit into
codex/issue-329-batch-lsoffrom
codex/issue-339-idempotent-install
Open

Make repeated exact installs idempotent#349
sdairs wants to merge 1 commit into
codex/issue-329-batch-lsoffrom
codex/issue-339-idempotent-install

Conversation

@sdairs

@sdairs sdairs commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • treat an exact ClickHouse version that is already installed as a successful no-op
  • emit the same already-installed message and --force hint used for partial version specs
  • cover both exact and partial local-first installs with subprocess regression tests

Why

The local-first install path special-cased exact version specs by returning VersionAlreadyInstalled, while partial specs returned the matching installed version successfully. That made equivalent idempotent install requests produce different exit codes and telemetry outcomes.

Impact

Repeated exact-version installs now exit 0 without resolving or downloading anything, so scripts can safely use local install to ensure a version is present. The separate output-copy cleanup remains tracked by #328.

Stack

This PR is stacked on #348 and should be reviewed as the delta from codex/issue-329-batch-lsof.

Closes #339.

Validation

  • cargo fmt --all --check
  • cargo build -p clickhousectl --all-targets
  • cargo test -p clickhousectl (571 tests)
  • cargo clippy -p clickhousectl --all-targets -- -D warnings

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes clickhousectl local install idempotent when the requested ClickHouse version is already installed, aligning behavior between exact (25.12.9.61) and partial (25.12) version specs in the local-first install path.

Changes:

  • Treat an already-installed exact version spec as a successful no-op (exit 0) in install_local_first, matching partial-spec behavior.
  • Reuse the existing “already installed” messaging (including the --force hint) for both exact and partial specs.
  • Add subprocess regression coverage for both partial and exact installed-version no-op installs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/clickhousectl/src/version_manager/install.rs Removes the exact-version “already installed” error in the local-first path so exact and partial specs both short-circuit successfully when a local match exists.
crates/clickhousectl/tests/local_install_local_first_test.rs Adds a shared subprocess helper and a new regression test covering exact-version no-op behavior alongside the existing partial-version coverage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sdairs
sdairs force-pushed the codex/issue-339-idempotent-install branch from a847a91 to 15edf26 Compare August 3, 2026 18:42
@sdairs
sdairs force-pushed the codex/issue-339-idempotent-install branch from 15edf26 to c275f1d Compare August 3, 2026 19:34
@sdairs
sdairs marked this pull request as ready for review August 3, 2026 19:37
@sdairs
sdairs requested review from iskakaushik and rndD as code owners August 3, 2026 19:37
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.

local install of an already-installed version: exact spelling errors (rc=1), partial spelling no-ops (rc=0)

2 participants