Add completion install command for shell autocompletion#148
Add completion install command for shell autocompletion#148Joeavaikath wants to merge 11 commits intomigtools:oadp-devfrom
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
Implements 'kubectl-oadp completion install' subcommand that automatically sets up shell completions for both oc and kubectl-oadp commands. Features: - Auto-detects shell type (bash/zsh/fish) from $SHELL - Generates completion files in parallel using errgroup - Validates bash-completion package presence for bash shells - Provides shell-specific setup instructions with smart RC file detection - Supports --shell flag to override detection - Supports --force flag to reinstall existing completions - Idempotent (safe to run multiple times) Implementation details: - Skips output wrapping for completion command to preserve stdout - Uses shellConfig map to eliminate duplicate shell-specific logic - Adds timeout protection for external commands (brew, completion generation) - Includes comprehensive design documentation Resolves: migtools#139 Signed-off-by: Joseph <jvaikath@redhat.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughDeleted Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "kubectl-oadp completion install"
participant ShellDetect as "Shell Detection"
participant Validator as "Validation (bash-completion check)"
participant Generator as "Completion Generation"
participant FS as "File System"
User->>CLI: run install [--shell <opt>]
CLI->>ShellDetect: detect or use provided shell
ShellDetect-->>CLI: shell (bash | zsh | fish)
CLI->>Validator: check existing installation & requirements
alt already installed && not --force
Validator-->>CLI: skip install
CLI->>User: print guidance
else
Validator->>Validator: if bash, verify bash-completion available
Validator-->>CLI: validation passed
CLI->>Generator: generate `oc` and `kubectl-oadp` completions (parallel)
Generator-->>CLI: completion scripts
CLI->>FS: create dirs & write files
FS-->>CLI: write success
CLI->>User: print success + shell-specific setup instructions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/design/139-COMPLETION_INSTALL_DESIGN.md (1)
38-47: Add language specifiers to fenced code blocks.Several code blocks are missing language specifiers, which affects rendering and syntax highlighting. Consider adding appropriate language identifiers.
📝 Proposed fixes
For line 38 (error message block):
-``` +```text Error: bash-completion package is required for bash shell completions.For line 170 (command structure):
-``` +```text kubectl-oadp completionFor line 180 (install flow):
-``` +```text 1. Detect shell (or use --shell flag)Also applies to: 170-176, 180-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/139-COMPLETION_INSTALL_DESIGN.md` around lines 38 - 47, The fenced code blocks in the design doc lack language specifiers; update each fenced block that contains the exact strings "Error: bash-completion package is required for bash shell completions.", "kubectl-oadp completion", and the numbered step "1. Detect shell (or use --shell flag)" to include an appropriate language tag (e.g., ```text or ```bash) so they render with proper syntax highlighting and formatting.cmd/completion/install.go (1)
89-98: Improve error messaging when shell cannot be detected.When
$SHELLis empty and--shellis not provided,o.Shellremains empty. The subsequentValidate()error message shows "unsupported shell: " with an empty string, which may confuse users.♻️ Proposed improvement
// Complete completes the options func (o *InstallOptions) Complete(args []string) error { // Auto-detect shell if not specified if o.Shell == "" { shell := os.Getenv("SHELL") if shell != "" { o.Shell = filepath.Base(shell) + } else { + return fmt.Errorf("could not auto-detect shell from $SHELL environment variable; use --shell to specify") } } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/completion/install.go` around lines 89 - 98, The Complete method leaves o.Shell empty if $SHELL and the --shell flag are missing, causing Validate() to emit "unsupported shell: " with a blank value; update func (o *InstallOptions) Complete to set a sensible default (e.g., o.Shell = "sh") when both the environment and flag are unset so Validate() receives a non-empty shell string (reference: InstallOptions.Complete and the o.Shell field used by Validate()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/completion/install.go`:
- Around line 206-215: getCompletionDir can return a path with no home prefix
when os.UserHomeDir() fails and $HOME is empty; update getCompletionDir to
return (string, error) (or at minimum return empty string on failure) and update
its callers (notably InstallOptions.Run) to check the returned error/empty
string and surface a descriptive error instead of proceeding; reference the
getCompletionDir function, InstallOptions.Run method, shellConfigs[o.Shell] and
config.completionSubdir when locating the code to change so you validate home
(from os.UserHomeDir() or $HOME) and abort with a clear error if it cannot be
determined.
- Around line 263-272: The code currently uses cmd.CombinedOutput() which mixes
stdout and stderr and can corrupt the generated completion file; change to use
cmd.Output() to capture only stdout and direct stderr to a separate buffer for
error diagnostics: create a bytes.Buffer (or similar) and assign it to
cmd.Stderr before calling cmd.Output(), call output, err := cmd.Output(), and on
error include the stderr buffer contents in the returned fmt.Errorf message
while still writing only the stdout bytes to os.WriteFile; references to update:
exec.CommandContext(ctx, command, "completion", o.Shell), cmd.CombinedOutput(),
and os.WriteFile(outputFile, output, 0644).
---
Nitpick comments:
In `@cmd/completion/install.go`:
- Around line 89-98: The Complete method leaves o.Shell empty if $SHELL and the
--shell flag are missing, causing Validate() to emit "unsupported shell: " with
a blank value; update func (o *InstallOptions) Complete to set a sensible
default (e.g., o.Shell = "sh") when both the environment and flag are unset so
Validate() receives a non-empty shell string (reference: InstallOptions.Complete
and the o.Shell field used by Validate()).
In `@docs/design/139-COMPLETION_INSTALL_DESIGN.md`:
- Around line 38-47: The fenced code blocks in the design doc lack language
specifiers; update each fenced block that contains the exact strings "Error:
bash-completion package is required for bash shell completions.", "kubectl-oadp
completion", and the numbered step "1. Detect shell (or use --shell flag)" to
include an appropriate language tag (e.g., ```text or ```bash) so they render
with proper syntax highlighting and formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 506b7248-2137-41ce-aceb-e7f271e17a0d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
COMPLETION.mdcmd/completion/completion.gocmd/completion/install.gocmd/root.godocs/design/139-COMPLETION_INSTALL_DESIGN.mddocs/design/kubectl-oadp-design.mdgo.mod
💤 Files with no reviewable changes (1)
- COMPLETION.md
Updates golangci-lint from v1.63.4 to v2.11.0 which is built with Go 1.26 and supports Go 1.25 projects. Changes installation method from 'go install' to official binary download script as v2.x requires. Adds .golangci.yml config to disable errcheck for CLI tool where output errors are typically unrecoverable (if writing to stdout fails, the program is fundamentally broken). Applies auto-fixes from new linter: - Use metav1.Time.Format() directly instead of .Time.Format() - Simplify variable declarations with type inference - Use fmt.Fprintf for string builder formatting Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
370-374:⚠️ Potential issue | 🟠 MajorDon't pipe a moving remote installer straight into
sh.Using
master/install.shmakes the bootstrap non-reproducible. Additionally, with make's default/bin/sh, a failedcurlis silently masked by the pipeline—if the download fails,sh -s --exits 0 on empty stdin, leaving$(GOLANGCI_LINT)absent while the recipe appears to succeed.Fetch a version-pinned script first (or vendor it) and execute it as a separate step to ensure failures are properly detected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 370 - 374, The Makefile currently pipes the remote installer into sh which masks curl failures and uses an unpinned master script; change the recipe that references GOLANGCI_LINT, GOLANGCI_LINT_VERSION and LOCALBIN to first download the specific, versioned installer to a temporary file with curl -fSLo (or vendor a copy), check curl's exit code, make the file executable, then execute that local script (chmod +x && ./script) so download failures are detected and the install is reproducible instead of using "curl | sh".
🧹 Nitpick comments (1)
.golangci.yml (1)
3-5: Scope theerrcheckexemption instead of disabling it repo-wide.The rationale here is about best-effort CLI output, but this switch also turns off ignored-error detection for unrelated file, process, and cleanup paths. A targeted
//nolint:errcheckor a narrower exclusion rule keeps that signal where it still matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 3 - 5, Remove the global errcheck disable in .golangci.yml and instead scope the exemption to the CLI codepaths: delete the "- errcheck" entry under linters.disable and add targeted suppressions either by placing //nolint:errcheck comments on the specific functions/files that intentionally ignore errors in your CLI (e.g., main, cmd/* handlers) or by adding golangci-lint exclude or linters-settings rules that match those CLI file patterns; keep the linter enabled for the rest of the repo so errcheck still flags ignored errors outside the intended CLI output paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 7-9: The CI lint workflow installs Go 1.24 while go.mod and
.golangci.yml target Go 1.25; update the workflow step that provisions Go (the
action input named "go-version" or the matrix entry used by the setup action in
the lint workflow) to use "1.25" so the runner Go version matches go.mod and
.golangci.yml and prevents mismatched-toolchain failures.
---
Outside diff comments:
In `@Makefile`:
- Around line 370-374: The Makefile currently pipes the remote installer into sh
which masks curl failures and uses an unpinned master script; change the recipe
that references GOLANGCI_LINT, GOLANGCI_LINT_VERSION and LOCALBIN to first
download the specific, versioned installer to a temporary file with curl -fSLo
(or vendor a copy), check curl's exit code, make the file executable, then
execute that local script (chmod +x && ./script) so download failures are
detected and the install is reproducible instead of using "curl | sh".
---
Nitpick comments:
In @.golangci.yml:
- Around line 3-5: Remove the global errcheck disable in .golangci.yml and
instead scope the exemption to the CLI codepaths: delete the "- errcheck" entry
under linters.disable and add targeted suppressions either by placing
//nolint:errcheck comments on the specific functions/files that intentionally
ignore errors in your CLI (e.g., main, cmd/* handlers) or by adding
golangci-lint exclude or linters-settings rules that match those CLI file
patterns; keep the linter enabled for the rest of the repo so errcheck still
flags ignored errors outside the intended CLI output paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 233bed93-8de5-45b8-a164-04f733601597
📒 Files selected for processing (8)
.golangci.ymlMakefilecmd/nabsl-request/describe.gocmd/nabsl-request/get.gocmd/non-admin/backup/describe.gocmd/non-admin/backup/get.gocmd/non-admin/restore/describe.gocmd/non-admin/restore/get.go
✅ Files skipped from review due to trivial changes (1)
- cmd/nabsl-request/get.go
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/lint.yml (1)
46-49: Redundant golangci-lint installation and potential version drift.The
golangci-lintis already installed and executed by the action on lines 19-22. Reinstalling it on line 48 is redundant and could install a different version than the action.Additionally,
version: lateston line 22 may drift from the v2.11.0 specified in the Makefile, causing inconsistent lint results between local development and CI. Consider pinning the version in the action to match.If the
unusedlinter is needed, enable it in.golangci.ymlrather than running a separate invocation.♻️ Suggested fix: pin version and remove redundant step
- name: golangci-lint uses: golangci/golangci-lint-action@v3 with: - version: latest + version: v2.11.0 args: --timeout=5m - name: Run go fmtThen remove the redundant step entirely and configure
unusedin.golangci.yml:- - name: Check for unused dependencies - run: | - go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest - golangci-lint run --enable unused🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint.yml around lines 46 - 49, Remove the redundant "name: Check for unused dependencies" step that runs "go install github.com/golangci/golangci-lint..." and "golangci-lint run --enable unused"; instead pin the golangci-lint action's "version: latest" to the Makefile's v2.11.0 so CI uses the same lint binary, and enable the "unused" linter in .golangci.yml (rather than calling "golangci-lint run --enable unused" separately) to avoid duplicate installs and version drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 46-49: Remove the redundant "name: Check for unused dependencies"
step that runs "go install github.com/golangci/golangci-lint..." and
"golangci-lint run --enable unused"; instead pin the golangci-lint action's
"version: latest" to the Makefile's v2.11.0 so CI uses the same lint binary, and
enable the "unused" linter in .golangci.yml (rather than calling "golangci-lint
run --enable unused" separately) to avoid duplicate installs and version drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48a57066-98a0-48e0-ab03-ef542790746d
📒 Files selected for processing (1)
.github/workflows/lint.yml
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Containerfile.download (1)
4-4: Pin the builder image to an immutable Go toolchain.
golang:1.25is a moving official tag; Docker Hub currently publishes both1.25and patch-specific tags like1.25.6, so rebuilds can silently pick up a different Go patch release over time. For a stage that produces release artifacts, pinning the exact patch or, ideally, the image digest will keep builds reproducible. (hub.docker.com)Suggested change
-FROM --platform=$BUILDPLATFORM golang:1.25 AS builder +FROM --platform=$BUILDPLATFORM golang:1.25.6 AS builder +# or pin by digest for full reproducibility: +# FROM --platform=$BUILDPLATFORM golang@sha256:<digest> AS builder🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Containerfile.download` at line 4, The builder stage uses a floating Go tag in the Dockerfile line "FROM --platform=$BUILDPLATFORM golang:1.25 AS builder", which makes builds non-reproducible; update that FROM line to pin to a specific patch release (e.g., golang:1.25.x) or, preferably, pin to the immutable image digest (sha256:...) for the chosen Go patch so the builder stage always uses the exact same toolchain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Containerfile.download`:
- Line 4: The builder stage uses a floating Go tag in the Dockerfile line "FROM
--platform=$BUILDPLATFORM golang:1.25 AS builder", which makes builds
non-reproducible; update that FROM line to pin to a specific patch release
(e.g., golang:1.25.x) or, preferably, pin to the immutable image digest
(sha256:...) for the chosen Go patch so the builder stage always uses the exact
same toolchain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0531c820-d64a-48a4-9b13-83a8d86a0881
📒 Files selected for processing (3)
.github/workflows/lint.yml.golangci.ymlContainerfile.download
🚧 Files skipped from review as they are similar to previous changes (2)
- .golangci.yml
- .github/workflows/lint.yml
1 participant
Why the changes were made
fixes #139
Completion Install Command (issue #139):
kubectl-oadp completion installto automate shell completion setup$SHELLenvironment variableocandkubectl-oadpcommandsbash-completionpackage is installed (hard requirement), shows install instructions if missing--forceflag usedDocumentation Refactor:
design/todocs/design/for better organizationdocs/design/139-COMPLETION_INSTALL_DESIGN.mdLinter Upgrade:
.golangci.ymlconfig to disableerrchecklinter (appropriate for CLI tools where output errors are unrecoverable)How to test the changes made
Build:
Test auto-detection and idempotency:
Test bash (with dependency check):
Test zsh:
Test fish:
Verify completions work (restart shell first):
Verify linter upgrade:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores