feat(cmd): add --runtime flag to init command#73
Conversation
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a Changes
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 Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/cmd/init.go (1)
147-151: Runtime validation relies on downstream manager.The runtime value is passed directly to
manager.Add()without pre-validation. This is acceptable sincemanager.Add()callsm.runtimeRegistry.Get(runtimeType)(per the context snippet frompkg/instances/manager.go:162) which will return an error if the runtime is invalid. However, consider whether early validation would provide a clearer error message to users.💡 Optional: Add early runtime validation for clearer error messages
You could validate the runtime earlier in
preRunto provide a more user-friendly error:// After determining runtime, validate it exists if _, err := manager.GetRuntime(i.runtime); err != nil { return outputErrorIfJSON(cmd, i.output, fmt.Errorf("invalid runtime %q: %w", i.runtime, err)) }This would require exposing a method to check runtime validity, but could improve the user experience with error messages like
invalid runtime "typo": runtime not foundinstead of the currentfailed to get runtime: ...message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init.go` around lines 147 - 151, The runtime is passed straight into i.manager.Add without early validation; add an early check (e.g., in preRun after resolving i.runtime) that verifies the runtime exists via the manager runtime lookup before calling i.manager.Add to yield a clearer error. Specifically, call the runtime validation method on the manager (e.g., manager.GetRuntime or a GetRuntime-like API) using i.runtime and return outputErrorIfJSON(cmd, i.output, fmt.Errorf("invalid runtime %q: %w", i.runtime, err)) on error so Add only runs for known runtimes; update preRun to perform this validation before proceeding to i.manager.Add.pkg/cmd/init_test.go (1)
468-522: Environment variable tests correctly avoid parallel execution.The tests using
t.Setenv()correctly avoid callingt.Parallel()as per the coding guideline. The nested subtest structure is appropriate for organizing related test cases.However, consider simplifying the nested structure. The outer
t.Runat line 468 creates a scope that only contains one innert.Run, which adds unnecessary nesting.💡 Optional: Flatten unnecessary nested test structure
- t.Run("uses environment variable when runtime flag is not provided", func(t *testing.T) { - // Note: Cannot use t.Parallel() when using t.Setenv() - - t.Run("with valid runtime from env", func(t *testing.T) { - t.Setenv("KORTEX_CLI_DEFAULT_RUNTIME", "fake") + t.Run("uses environment variable when runtime flag is not provided", func(t *testing.T) { + // Note: Cannot use t.Parallel() when using t.Setenv() + t.Setenv("KORTEX_CLI_DEFAULT_RUNTIME", "fake") - tempDir := t.TempDir() + tempDir := t.TempDir() // ... rest of test - }) }) - t.Run("runtime flag takes precedence over environment variable", func(t *testing.T) { - // Note: Cannot use t.Parallel() when using t.Setenv() - - t.Run("flag overrides env", func(t *testing.T) { - t.Setenv("KORTEX_CLI_DEFAULT_RUNTIME", "env-runtime") + t.Run("runtime flag takes precedence over environment variable", func(t *testing.T) { + // Note: Cannot use t.Parallel() when using t.Setenv() + t.Setenv("KORTEX_CLI_DEFAULT_RUNTIME", "env-runtime") - tempDir := t.TempDir() + tempDir := t.TempDir() // ... rest of test - }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init_test.go` around lines 468 - 522, The outer wrapper t.Run that only contains a single nested t.Run is unnecessary; simplify the tests by removing the redundant outer t.Run and promote the inner subtests (e.g., "with valid runtime from env" and "flag overrides env") to top-level subtests so each directly calls t.Setenv, creates initCmd{runtime: ...}, constructs the cobra.Command with the same flags and tempDir, and invokes c.preRun; keep the existing assertions that c.runtime equals the expected value and retain the comment about not using t.Parallel() when using t.Setenv(); references: tests exercising initCmd.preRun and the subtest names "with valid runtime from env" and "flag overrides env".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/init_test.go`:
- Around line 468-522: The outer wrapper t.Run that only contains a single
nested t.Run is unnecessary; simplify the tests by removing the redundant outer
t.Run and promote the inner subtests (e.g., "with valid runtime from env" and
"flag overrides env") to top-level subtests so each directly calls t.Setenv,
creates initCmd{runtime: ...}, constructs the cobra.Command with the same flags
and tempDir, and invokes c.preRun; keep the existing assertions that c.runtime
equals the expected value and retain the comment about not using t.Parallel()
when using t.Setenv(); references: tests exercising initCmd.preRun and the
subtest names "with valid runtime from env" and "flag overrides env".
In `@pkg/cmd/init.go`:
- Around line 147-151: The runtime is passed straight into i.manager.Add without
early validation; add an early check (e.g., in preRun after resolving i.runtime)
that verifies the runtime exists via the manager runtime lookup before calling
i.manager.Add to yield a clearer error. Specifically, call the runtime
validation method on the manager (e.g., manager.GetRuntime or a GetRuntime-like
API) using i.runtime and return outputErrorIfJSON(cmd, i.output,
fmt.Errorf("invalid runtime %q: %w", i.runtime, err)) on error so Add only runs
for known runtimes; update preRun to perform this validation before proceeding
to i.manager.Add.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d8c7556-0ad8-4a6e-be2b-2768390d4947
📒 Files selected for processing (3)
README.mdpkg/cmd/init.gopkg/cmd/init_test.go
feloy
changed the title
Add mandatory --runtime flag to the init command for selecting the runtime implementation when registering workspaces. The runtime can be specified via: - --runtime/-r flag (highest priority) - KORTEX_CLI_DEFAULT_RUNTIME environment variable - Error if neither is set (runtime is required) Updated README.md with comprehensive Environment Variables section documenting KORTEX_CLI_DEFAULT_RUNTIME and KORTEX_CLI_STORAGE, including usage examples and priority order. Resolves kortex-hub#51 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3 participants
Add mandatory --runtime flag to the init command for selecting the runtime implementation when registering workspaces. The runtime can be specified via:
Updated README.md with comprehensive Environment Variables section documenting KORTEX_CLI_DEFAULT_RUNTIME and KORTEX_CLI_STORAGE, including usage examples and priority order.
Resolves #51