feat(registry): provide directory to runtimes#71
Merged
feloy merged 1 commit intokortex-hub:mainfrom Mar 16, 2026
Merged
Conversation
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
feloy
changed the title
📝 WalkthroughWalkthroughThis pull request implements directory provisioning for runtimes by introducing a StorageAware interface that allows runtimes to receive and initialize with dedicated storage directories. NewRegistry is refactored to accept a storage directory parameter, create runtime-specific subdirectories, and invoke Initialize on StorageAware implementations during registration. Changes
Sequence DiagramsequenceDiagram
participant Manager as Manager.NewManager()
participant Registry as Registry.NewRegistry()
participant Storage as Storage Directory
participant Runtime as Runtime.Register()
Manager->>Manager: Construct runtimesDir<br/>(storageDir/runtimes)
Manager->>Registry: NewRegistry(runtimesDir)
activate Registry
Registry->>Storage: Validate & create runtimesDir
Storage-->>Registry: Directory ready
Registry-->>Manager: Registry instance
deactivate Registry
Manager->>Runtime: Register(runtime)
activate Runtime
alt Runtime implements StorageAware
Runtime->>Storage: Create runtimeType subdirectory
Storage-->>Runtime: Subdirectory created
Runtime->>Runtime: Initialize(runtimeTypeDir)
Runtime-->>Manager: Initialization complete
else Non-StorageAware runtime
Runtime-->>Manager: Registration complete
end
deactivate Runtime
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 |
![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/runtime/registry_test.go (1)
342-376: Consider adding a compile-time interface check for storageAwareRuntime.The
storageAwareRuntimestruct implements bothRuntimeandStorageAwareinterfaces. Following the Module Design Pattern used elsewhere in the codebase, consider adding compile-time checks.🔧 Proposed addition after storageAwareRuntime definition
type storageAwareRuntime struct { typeID string storageDir string initializeErr error } + +// Compile-time checks +var _ Runtime = (*storageAwareRuntime)(nil) +var _ StorageAware = (*storageAwareRuntime)(nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/registry_test.go` around lines 342 - 376, Add compile-time interface assertions to ensure storageAwareRuntime implements the expected interfaces: add assertions referencing storageAwareRuntime, Runtime, and StorageAware (e.g., assign (*storageAwareRuntime)(nil) to variables typed as Runtime and StorageAware) immediately after the storageAwareRuntime definition so the compiler will catch any future signature drift.pkg/runtime/registry.go (1)
63-71: Consider adding a compile-time interface check for StorageAware.The codebase follows the Module Design Pattern with compile-time interface checks (see line 81 for
Registry). Consider adding a similar check forStorageAwareto catch implementation mismatches early, especially since runtimes implementing this interface will be in separate packages.🔧 Proposed addition after StorageAware definition
// Ensure storageAwareRuntime implements StorageAware at compile time (example for implementors) // var _ StorageAware = (*yourRuntime)(nil)Note: Since this is an optional interface for external implementors, the compile-time check would be in the implementing packages rather than here. This is acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/runtime/registry.go` around lines 63 - 71, Add a compile-time interface assertion for StorageAware by placing a var _ StorageAware = (*yourRuntime)(nil) in the concrete runtime implementation package (not in this file); alternatively add a short example comment immediately after the StorageAware type declaration showing that pattern. Reference the StorageAware interface and the concrete runtime type name (e.g., yourRuntime) so implementors get an early compile-time failure if their type signature doesn't satisfy StorageAware.
🤖 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/runtime/registry_test.go`:
- Around line 342-376: Add compile-time interface assertions to ensure
storageAwareRuntime implements the expected interfaces: add assertions
referencing storageAwareRuntime, Runtime, and StorageAware (e.g., assign
(*storageAwareRuntime)(nil) to variables typed as Runtime and StorageAware)
immediately after the storageAwareRuntime definition so the compiler will catch
any future signature drift.
In `@pkg/runtime/registry.go`:
- Around line 63-71: Add a compile-time interface assertion for StorageAware by
placing a var _ StorageAware = (*yourRuntime)(nil) in the concrete runtime
implementation package (not in this file); alternatively add a short example
comment immediately after the StorageAware type declaration showing that
pattern. Reference the StorageAware interface and the concrete runtime type name
(e.g., yourRuntime) so implementors get an early compile-time failure if their
type signature doesn't satisfy StorageAware.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07e7b284-ae5b-45a8-85f7-f47a9f6af380
📒 Files selected for processing (4)
pkg/instances/manager.gopkg/instances/manager_test.gopkg/runtime/registry.gopkg/runtime/registry_test.go
jeffmaury
approved these changes
Mar 16, 2026
| } | ||
|
|
||
| // Create storage directory if it doesn't exist | ||
| if err := os.MkdirAll(absStorageDir, 0755); err != nil { |
Contributor
There was a problem hiding this comment.
question: do we need the execution bit ?
Contributor
Author
There was a problem hiding this comment.
For the user to be able to 'cd' to it, yes
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
3 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.
Optionally provide storage directory to runtimes
Fixes #66