feat(runtime): add centralized runtime registration system#78
feat(runtime): add centralized runtime registration system#78feloy merged 1 commit intokortex-hub:mainfrom
Conversation
Created pkg/runtimesetup package to abstract runtime registration from individual commands. Commands now call RegisterAll() instead of manually registering each runtime, making it easier to add new runtime implementations without modifying existing commands. Added /add-runtime skill with step-by-step documentation for creating new runtime implementations. Updated command creation skills to include guidance on when runtime registration is needed. 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! |
📝 WalkthroughWalkthroughIntroduces a pluggable runtime registration system with interfaces and a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/runtimesetup/register_test.go (1)
72-163: Tests are comprehensive, consider adding a case for runtimes withoutAvailableinterface.The current tests cover the main scenarios well. One edge case not explicitly tested: a runtime that doesn't implement the
Availableinterface should always be registered. The currenttestRuntimealways implementsAvailable, so this path isn't directly tested.💡 Optional test case for runtimes without Available interface
// simpleRuntime doesn't implement Available interface type simpleRuntime struct { runtimeType string } func (s *simpleRuntime) Type() string { return s.runtimeType } func (s *simpleRuntime) Create(ctx context.Context, params runtime.CreateParams) (runtime.RuntimeInfo, error) { return runtime.RuntimeInfo{}, fmt.Errorf("not implemented") } // ... other required methods t.Run("registers runtimes without Available interface", func(t *testing.T) { t.Parallel() registrar := &fakeRegistrar{} testFactories := []runtimeFactory{ func() runtime.Runtime { return &simpleRuntime{runtimeType: "simple"} }, } err := registerAllWithAvailable(registrar, testFactories) if err != nil { t.Fatalf("registerAllWithAvailable() failed: %v", err) } if len(registrar.registered) != 1 { t.Errorf("Expected 1 runtime to be registered, got %d", len(registrar.registered)) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtimesetup/register_test.go` around lines 72 - 163, Add a test case to assert that runtimes which do not implement the Available interface are always registered: create a minimal simpleRuntime type (implementing only runtime.Runtime methods like Type and Create but not Available), add a t.Run block similar to the other table tests that uses fakeRegistrar and a factory returning &simpleRuntime{runtimeType: "simple"}, call registerAllWithAvailable(registrar, testFactories) and assert no error and that fakeRegistrar.registered length is 1 and contains "simple"; reference test symbols testRuntime, simpleRuntime, fakeRegistrar, and registerAllWithAvailable when locating where to add the new test.
🤖 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/runtimesetup/register.go`:
- Around line 55-59: availableRuntimes currently includes fake.New which is a
test-only runtime and because fakeRuntime doesn't implement the Available
interface it will always be registered via RegisterAll(); fix by either removing
fake.New from the availableRuntimes slice or implementing the
Available(interface) method on the fake runtime type (e.g., fakeRuntime) to
return false so RegisterAll() skips it—locate the availableRuntimes declaration
and the fake package (fake.New / fakeRuntime) and apply one of these two
changes.
In `@skills/add-runtime/SKILL.md`:
- Around line 34-37: The runtime template import block is missing the os/exec
package used by exec.LookPath, so add the import "os/exec" to the import list in
the SKILL.md runtime snippet where the imports include "context" and
"github.com/kortex-hub/kortex-cli/pkg/runtime"; also apply the same import
addition to the other snippet around lines 66–72 that uses exec.LookPath so the
examples compile.
---
Nitpick comments:
In `@pkg/runtimesetup/register_test.go`:
- Around line 72-163: Add a test case to assert that runtimes which do not
implement the Available interface are always registered: create a minimal
simpleRuntime type (implementing only runtime.Runtime methods like Type and
Create but not Available), add a t.Run block similar to the other table tests
that uses fakeRegistrar and a factory returning &simpleRuntime{runtimeType:
"simple"}, call registerAllWithAvailable(registrar, testFactories) and assert no
error and that fakeRegistrar.registered length is 1 and contains "simple";
reference test symbols testRuntime, simpleRuntime, fakeRegistrar, and
registerAllWithAvailable when locating where to add the new test.
🪄 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: 44927e3e-cb0e-401a-8190-e20142190d4c
📒 Files selected for processing (11)
.claude/skills/add-runtimeAGENTS.mdpkg/cmd/init.gopkg/cmd/workspace_remove.gopkg/cmd/workspace_start.gopkg/cmd/workspace_stop.gopkg/runtimesetup/register.gopkg/runtimesetup/register_test.goskills/add-command-simple/SKILL.mdskills/add-command-with-json/SKILL.mdskills/add-runtime/SKILL.md
Created pkg/runtimesetup package to abstract runtime registration from individual commands. Commands now call RegisterAll() instead of manually registering each runtime, making it easier to add new runtime implementations without modifying existing commands.
Added /add-runtime skill with step-by-step documentation for creating new runtime implementations. Updated command creation skills to include guidance on when runtime registration is needed.
Fixes #76