feat(completion): add autocompletion for IDs, output and runtime#86
Open
feloy wants to merge 5 commits intokortex-hub:mainfrom
Open
feat(completion): add autocompletion for IDs, output and runtime#86feloy wants to merge 5 commits intokortex-hub:mainfrom
feloy wants to merge 5 commits intokortex-hub:mainfrom
Conversation
Adds intelligent shell completion that filters workspace IDs based on their runtime state. The start and remove commands show only non-running workspaces, while stop shows only running workspaces. This prevents users from attempting invalid operations and improves the CLI experience. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds configurable shell completion for the --output flag. Each command can specify its own list of valid output formats through the newOutputFlagCompletion factory. Currently all commands support only 'json', but the design allows easy extension for future formats. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds shell completion for the --runtime flag that lists all available runtimes. The completion automatically filters out the "fake" runtime which is only used for testing. Also adds ListRuntimes() method to the Manager interface to expose registered runtime types. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds Cobra shell autocompletion helpers for workspace ID arguments (filtered by runtime state), output and runtime flag value completions, integrates those into several workspace/alias commands, and exposes a runtimesetup.ListAvailable() helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Shell as "Shell/Cobra"
participant Cmd as "cobra.Command (flag/arg completion)"
participant AC as "autocomplete helper"
participant FS as "Filesystem (storage dir)"
participant IM as "instances.Manager"
participant RS as "runtimesetup"
Shell->>Cmd: user requests completion
Cmd->>AC: invoke ValidArgsFunction / FlagCompletion
AC->>Cmd: read global `--storage` flag
Cmd-->>AC: flag value
AC->>FS: resolve & stat storage path
FS-->>AC: exists? / error
alt storage exists
AC->>IM: NewManager(storage)
IM-->>AC: manager
AC->>IM: List instances
IM-->>AC: instances list
AC->>AC: filter by state predicate (running/non-running)
AC->>Cmd: return completions + ShellCompDirectiveNoFileComp
else storage missing or errors
AC->>Cmd: return [] + ShellCompDirectiveNoFileComp / ShellCompDirectiveError
end
note over Cmd,RS: separate flow for `--runtime` completion
Cmd->>RS: ListAvailable()
RS-->>Cmd: available runtimes (excluding "fake")
Cmd->>Shell: runtime completions + ShellCompDirectiveNoFileComp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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)
pkg/cmd/autocomplete_test.go (2)
424-447: Test only verifies negative case, not positive case.This test confirms that
"fake"is excluded from completions but doesn't verify that legitimate runtimes are returned. Since no runtimes are registered in the test setup,completionsmay be empty, making the "fake not in completions" check vacuously true.Consider registering at least one non-fake runtime and verifying it appears in completions to strengthen this test.
💡 Suggested approach
t.Run("returns available runtimes excluding fake", func(t *testing.T) { t.Parallel() storageDir := t.TempDir() + // Create manager and register runtimes + manager, err := instances.NewManager(storageDir) + if err != nil { + t.Fatalf("failed to create manager: %v", err) + } + // Register fake runtime (should be excluded) + if err := manager.RegisterRuntime(fake.New()); err != nil { + t.Fatalf("failed to register fake runtime: %v", err) + } + // TODO: Register a real runtime (e.g., podman) to verify it IS returned // Create a command with the storage flag cmd := &cobra.Command{} cmd.Flags().String("storage", storageDir, "test storage flag") // Call completion function completions, directive := completeRuntimeFlag(cmd, []string{}, "") // Verify "fake" is not in the completions for _, completion := range completions { if completion == "fake" { t.Errorf("Expected 'fake' runtime to be filtered out, but it was included") } } + // TODO: Verify that registered non-fake runtimes ARE in completions + // if len(completions) == 0 { + // t.Errorf("Expected at least one runtime completion") + // } // Verify directive if directive != cobra.ShellCompDirectiveNoFileComp { t.Errorf("Expected ShellCompDirectiveNoFileComp, got %v", directive) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/autocomplete_test.go` around lines 424 - 447, The test only checks that "fake" is absent but never ensures any real runtimes are returned; update the test that calls completeRuntimeFlag to register at least one real runtime before invoking it (e.g., create/register a runtime named "node" or "docker" via the same runtime registration API used in production), then assert that the expected runtime appears in the completions and that "fake" is not present, keeping the existing checks for the directive and using the same cmd with cmd.Flags().String("storage", storageDir, ...).
136-155: Test name and setup are misaligned.The test is named "returns empty list when list fails" but the setup doesn't actually cause the list operation to fail—it uses a valid temp directory with no corrupted data. The comment on lines 140-141 mentions "List() may fail on corrupted data" but no corruption is introduced.
Additionally, the assertion on lines 152-154 is very permissive, accepting either empty completions OR an error directive, which doesn't match the specific behavior described in the test name.
Consider either:
- Renaming to "returns empty list when no instances exist" (which matches what's actually tested), or
- Actually inducing a failure condition (e.g., corrupting the storage directory structure)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/autocomplete_test.go` around lines 136 - 155, The test name and assertions don't match the setup: the test uses a valid temp storageDir and doesn't induce List() failure for completeNonRunningWorkspaceID, so either rename the t.Run to "returns empty list when no instances exist" and update the comment and assertion to expect len(completions)==0 and directive==cobra.ShellCompDirectiveNoFileComp, or actually cause List() to fail (e.g., create storageDir then write corrupt data or invalid permissions before calling completeNonRunningWorkspaceID) and keep the current name but assert that directive==cobra.ShellCompDirectiveError; update related variables (storageDir, cmd) and comments accordingly to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/autocomplete.go`:
- Around line 21-27: The completion handlers are calling instances.NewManager(),
which creates storage dirs; change the completion code to avoid creating on-disk
state by checking whether the storage path exists before instantiating a
manager. Specifically, in the autocomplete command completion functions that
call instances.NewManager(), first query the runtime storage path (via
runtimesetup or the same config source used elsewhere) and if the path does not
exist/is empty, short-circuit and return no suggestions; otherwise only then
call instances.NewManager(). Alternatively, if instances.NewManager supports a
non-creating option, use that (e.g., a constructor flag like "no-create");
update all occurrences (the calls around instances.NewManager() noted in the
review) to apply this guard so tab-completion never creates directories.
In `@pkg/instances/manager_test.go`:
- Around line 1614-1617: The test currently asserts the fake runtime by checking
runtimes[0] which is order-dependent; change the assertion to check that the
"runtimes" slice contains "fake" (e.g., iterate runtimes or use a helper
contains function) and fail the test if not found so the check is
order-independent—update the assertion around the runtimes variable in the test
(the block that currently does if len(runtimes) > 0 && runtimes[0] != "fake") to
verify containment instead.
---
Nitpick comments:
In `@pkg/cmd/autocomplete_test.go`:
- Around line 424-447: The test only checks that "fake" is absent but never
ensures any real runtimes are returned; update the test that calls
completeRuntimeFlag to register at least one real runtime before invoking it
(e.g., create/register a runtime named "node" or "docker" via the same runtime
registration API used in production), then assert that the expected runtime
appears in the completions and that "fake" is not present, keeping the existing
checks for the directive and using the same cmd with
cmd.Flags().String("storage", storageDir, ...).
- Around line 136-155: The test name and assertions don't match the setup: the
test uses a valid temp storageDir and doesn't induce List() failure for
completeNonRunningWorkspaceID, so either rename the t.Run to "returns empty list
when no instances exist" and update the comment and assertion to expect
len(completions)==0 and directive==cobra.ShellCompDirectiveNoFileComp, or
actually cause List() to fail (e.g., create storageDir then write corrupt data
or invalid permissions before calling completeNonRunningWorkspaceID) and keep
the current name but assert that directive==cobra.ShellCompDirectiveError;
update related variables (storageDir, cmd) and comments accordingly to reflect
the chosen approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12fc07cf-740b-4fa7-91fc-8e3e2a849e9c
📒 Files selected for processing (12)
pkg/cmd/autocomplete.gopkg/cmd/autocomplete_test.gopkg/cmd/init.gopkg/cmd/remove.gopkg/cmd/start.gopkg/cmd/stop.gopkg/cmd/workspace_list.gopkg/cmd/workspace_remove.gopkg/cmd/workspace_start.gopkg/cmd/workspace_stop.gopkg/instances/manager.gopkg/instances/manager_test.go
Tab-completion was calling instances.NewManager() which creates storage directories as a side effect. Modified completion handlers to check if storage exists before instantiating a manager for workspace IDs, and added runtimesetup.ListAvailable() for runtime flag completion that doesn't require a manager instance. Removed the now-unused Manager.ListRuntimes() method. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/autocomplete_test.go`:
- Around line 136-154: The test uses t.TempDir() which exists so it won't
simulate a failing List; change storageDir to a non-existent path (e.g.,
filepath.Join(t.TempDir(), "missing")) so manager/List will fail when
completeNonRunningWorkspaceID is called, and tighten the expectation to assert a
deterministic failure: require completions to be empty and directive to equal
cobra.ShellCompDirectiveError; update the test setup where
cmd.Flags().String("storage", storageDir, ...) and the final assertion to check
both len(completions) == 0 and directive == cobra.ShellCompDirectiveError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbdbe2a3-1ddd-446d-8ab7-20ac00eb303e
📒 Files selected for processing (3)
pkg/cmd/autocomplete.gopkg/cmd/autocomplete_test.gopkg/runtimesetup/register.go
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
2 participants
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.
Adds intelligent shell completion that filters workspace IDs based on their runtime state. The start and remove commands show only non-running workspaces, while stop shows only running workspaces. This prevents users from attempting invalid operations and improves the CLI experience.
Adds configurable shell completion for the --output flag. Each command can specify its own list of valid output formats through the newOutputFlagCompletion factory. Currently all commands support only 'json', but the design allows easy extension for future formats.
Adds shell completion for the --runtime flag that lists all available runtimes. The completion automatically filters out the "fake" runtime which is only used for testing. Also adds ListRuntimes() method to the Manager interface to expose registered runtime types.
Fixes #85