CFX-7958: fix(install): wrong completion command - #884
Conversation
ajalon1
changed the title
|
/approve-install-tests |
|
oh actually I think there's a way to read the names from the cobra command tree |
…nclude "self" Every user-facing hint in cmd/self/completion/install/cmd.go and cmd/self/completion/uninstall/cmd.go (Example blocks, the "already installed" reinstall hint, and the dry-run hints) told users to run `dr completion install/uninstall ...`, but those commands only exist under `dr self completion install/uninstall ...`. Copy-pasting the hint failed with an unknown-command error. install/cmd.go line 354 already had the correct `self` prefix; the rest did not. The two runtime-printed hints (reinstall and dry-run, in both files) now derive their command path from cmd.CommandPath() instead of a hardcoded string, so they can't silently drift out of sync again if the command is ever renamed or reparented.
a828d93 to
f66e7e8
Compare
| By default, this command runs in preview mode. Use '--yes' to install directly.`, | ||
| Example: ` # Preview what would be installed (default behavior): | ||
| ` + version.CliName + ` completion install | ||
| ` + version.CliName + ` self completion install |
There was a problem hiding this comment.
I thought about tweaking the Example strings to also use the updated commands, but the strings get registered on the command well before the command gets wired up, so ... I'll take the small loss if we ever have to move self completion again. The more impt thing is taht the actual code is up to date
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
❌ Some smoke tests failed. (Fork PR) ❌ Security Scan: failure |
chasdr
left a comment
chasdr
left a comment
There was a problem hiding this comment.
👋 chasbot here — chas sent me to second-guess whatever your droid shipped, and the audit came back annoyingly boring.
what held up under scrutiny:
- premise checks out: root_factory.go:390 disables cobra's default completion command, so every hint this fixes really was a dead instruction
- CommandPath() is the right fix for the two runtime hints — they can't drift again. the Example blocks staying hardcoded is also correct, since they're built before the command is wired to its parents
- getInstallFunc / installFunc still receive cmd.Root(), so completion generation is untouched
- the tests wire the real parent chain and assert on rendered output. ran them plus task lint (all three GOOS) locally: green
one thing your droid walked right past — to be fair, so has everyone since completion moved under self, it's pre-existing and not this PR's problem: the powershell installer writes dr completion powershell | Out-String | Invoke-Expression into the user's profile (install/cmd.go L479-L491). the exact disease this PR cures everywhere else, except this copy executes at every powershell startup, so completions never load there at all. not a one-liner either: the already-installed guard and the uninstall detection both match the literal dr completion powershell string, so a fix has to handle old profiles too. follow-up ticket material.
minor: runInstall's doc comment is 5 lines saying the CommandPath rationale twice, and the same rationale repeats in runUninstall and three test comments. chas runs a 2-line comment budget on me — you get it as a suggestion, not a hook.
one dedup nit inline. approving — this stands on its own.
| // cmd/self/completion/cmd.go), so cmd.CommandPath() resolves exactly as it | ||
| // does at runtime. This package can't import cmd/self/completion directly — | ||
| // that package imports this one, so doing so would create an import cycle. | ||
| func wireCommandPath(cmd *cobra.Command) { |
There was a problem hiding this comment.
wireCommandPath + captureStdout are byte-identical to the install test's copies, and internal/testutil is already imported here. worth moving them there?
Move shared completion test helpers into internal/testutil and reuse them from install and uninstall tests. Trim repeated CommandPath rationale comments and reuse the existing test home helper in the PowerShell profile test. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
❌ Some smoke tests failed. (Fork PR) ❌ Security Scan: failure |
Bump golang.org/x/crypto to v0.55.0 to resolve the Trivy finding reported by the fork smoke workflow. go mod tidy also updates golang.org/x/net to v0.57.0. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @ajalon1 What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
2 participants
Note
Low Risk
User-facing CLI messaging and help examples only; no changes to install/uninstall behavior or security-sensitive paths.
Overview
Fixes CFX-7958, where reinstall and dry-run messages suggested
dr completion install(and similar) even though the real command isdr self completion install.Install and uninstall now pass the invoked
*cobra.Commandinto the hint helpers so copy usescmd.CommandPath()instead of a hardcodedCliName + " completion …"string. Completion generation still usescmd.Root()where needed. Help examples are updated to show theself completionpath.Tests add a synthetic parent chain (
wireCommandPath), stdout capture, and assertions that printed hints matchdr self completion install|uninstall ….Reviewed by Cursor Bugbot for commit 4f19d5b. Configure here.