Make repeated exact installs idempotent - #349
Open
sdairs wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
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
--forcehint) 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
force-pushed
the
codex/issue-339-idempotent-install
branch
from
August 3, 2026 18:42
a847a91 to
15edf26
Compare
sdairs
force-pushed
the
codex/issue-339-idempotent-install
branch
from
August 3, 2026 19:34
15edf26 to
c275f1d
Compare
sdairs
marked this pull request as ready for review
August 3, 2026 19:37
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--forcehint used for partial version specsWhy
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 installto 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 --checkcargo build -p clickhousectl --all-targetscargo test -p clickhousectl(571 tests)cargo clippy -p clickhousectl --all-targets -- -D warnings