CNTRLPLANE-1691: Add plugin system with OADP support #7188
Conversation
Add a comprehensive plugin system that allows enabling/disabling CLI functionality without affecting CI operations. Plugins are disabled by default and configured via ~/.config/hcp/config.yaml. Features: - Plugin interface with registration system - Configuration management with YAML persistence - Plugin enable/disable commands (hypershift plugins enable/disable) - Plugin listing command (hypershift plugins list) - OADP plugin implementation as example - Function injection to avoid import cycles The plugin system provides: - Consistent configuration pattern for all plugins - Central API for plugin state management - Comprehensive test coverage with Gomega - Public API functions for external usage BREAKING CHANGE: Adds new 'plugins' subcommand to both hypershift and hcp CLIs. The OADP backup functionality will be moved behind this plugin system in future commits. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Juan Manuel Parrilla Madrid <[email protected]>
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-1691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jparrill: This pull request references CNTRLPLANE-1691 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a comprehensive plugin management system for the HyperShift CLI, including a plugin registry, YAML-based configuration for persistence, enable/disable functionality, and an OADP plugin implementation. Integrates list, enable, and disable commands with corresponding tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cmd/plugins/registry.go (2)
3-8: Document thread-safety expectations and consider defensive copying.The global
registeredPluginsslice is modified byRegisterPluginwithout synchronization. While this appears safe for the current init-time registration pattern (as seen in loader.go), the public API doesn't prevent misuse.Consider:
- Adding documentation that
RegisterPluginshould only be called during package initialization.- Protecting against concurrent modification if the API might be used outside init contexts.
Example documentation:
+// RegisterPlugin adds a plugin to the global registry. +// This function should only be called during package initialization (init functions). +// It is not safe for concurrent use. func RegisterPlugin(plugin Plugin) { registeredPlugins = append(registeredPlugins, plugin) }
11-13: Consider returning a copy to prevent external mutation.
GetAllPluginsreturns a direct reference to the internal slice, allowing callers to potentially modify the registry. For a "Chill" review, this is acceptable if internal use is controlled, but consider defensive copying for a more robust public API.Optional improvement:
func GetAllPlugins() []Plugin { - return registeredPlugins + result := make([]Plugin, len(registeredPlugins)) + copy(result, registeredPlugins) + return result }cmd/plugins/oadp/oadp_test.go (1)
26-39: Test assertion could be strengthened.The test uses
BeAssignableToTypeOf(true)which only verifies the return type is boolean, not that the configuration system works correctly. While this is acceptable for a foundation PR, consider adding integration tests later that verify the actual enablement behavior with mocked or test configuration.cmd/plugins/loader_test.go (1)
125-133: Minor: Comments should be in English.Lines 125 and 130 contain Spanish comments. For consistency with the codebase, please use English.
- // Verificar el primer plugin (enabled) + // Verify the first plugin (enabled) g.Expect(status[0].Name).To(Equal("enabled-plugin")) g.Expect(status[0].Description).To(Equal("This is an enabled plugin")) g.Expect(status[0].Enabled).To(BeTrue()) - // Verificar el segundo plugin (disabled) + // Verify the second plugin (disabled) g.Expect(status[1].Name).To(Equal("disabled-plugin"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (13)
cmd/plugins/command.go(1 hunks)cmd/plugins/command_test.go(1 hunks)cmd/plugins/config.go(1 hunks)cmd/plugins/config_test.go(1 hunks)cmd/plugins/loader.go(1 hunks)cmd/plugins/loader_test.go(1 hunks)cmd/plugins/oadp/oadp.go(1 hunks)cmd/plugins/oadp/oadp_test.go(1 hunks)cmd/plugins/registry.go(1 hunks)cmd/plugins/registry_test.go(1 hunks)cmd/plugins/types.go(1 hunks)main.go(2 hunks)product-cli/main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
🔇 Additional comments (24)
main.go (1)
31-31: LGTM! Clean integration of the plugins command.The plugin system is correctly integrated following the established pattern for other subcommands.
Also applies to: 69-69
product-cli/main.go (1)
24-24: LGTM! Consistent integration across both CLIs.The plugin command is correctly added to the hcp CLI, maintaining consistency with the hypershift CLI.
Also applies to: 50-50
cmd/plugins/oadp/oadp_test.go (1)
57-60: TODO appropriately flagged for future work.The placeholder implementation is clearly documented. This is appropriate for a foundation PR.
cmd/plugins/types.go (2)
8-13: Well-defined plugin interface.The interface is clean and appropriate for the current foundation.
RegisterCreateCommandsis specific to the create command, which aligns with the PR's immediate goal of moving OADP backup functionality behind the plugin system. If future plugins need to extend other commands, the interface can be evolved incrementally.
16-20: LGTM! Simple and clear status representation.The PluginStatus struct provides a clean snapshot of plugin state for reporting purposes.
cmd/plugins/loader_test.go (1)
14-19: Good test isolation pattern.The backup/restore pattern for global state ensures test isolation. Since tests don't use
t.Parallel(), there are no race conditions.cmd/plugins/loader.go (3)
9-12: Excellent use of function injection to avoid import cycles.The pattern of passing
IsPluginEnabledto the OADP plugin constructor avoids circular dependencies while maintaining clean separation of concerns.
15-20: Clean implementation with implicit error handling.
LoadPluginsIntoCreateCommandassumesRegisterCreateCommandsnever fails. This is acceptable for the current scope, as command registration failures are typically programming errors that would surface during development/testing.
23-33: LGTM! Straightforward status aggregation.The function cleanly transforms plugin state into a reportable format.
cmd/plugins/registry_test.go (2)
12-34: Well-designed mock plugin for testing.The mock implementation provides clear verification points (e.g.,
commandsRegistered) for testing plugin behavior.
36-150: Comprehensive test coverage for the plugin registry.The test suite covers:
- Registration and ordering
- Retrieval of all plugins
- Filtering by enabled state
- Edge cases (empty registry, all disabled)
The backup/restore pattern ensures proper test isolation.
cmd/plugins/config_test.go (1)
11-207: LGTM! Comprehensive test coverage for the config module.The test suite properly covers all configuration management functions with good isolation practices:
- Uses
t.TempDir()for filesystem isolation- Mocks
getConfigPathto avoid side effects- Resets global state between tests
- Validates both success paths and edge cases (missing plugins, default config, etc.)
- Uses table-driven tests where appropriate
cmd/plugins/command.go (3)
12-56: LGTM! Clean command structure.The command hierarchy is well-organized with appropriate argument validation using
cobra.ExactArgs(1)for enable/disable commands.
58-78: LGTM! Clean list implementation.The function properly handles the empty plugin case and uses
tabwriterfor aligned output.
124-137: LGTM! Clean public API wrappers.These functions provide a consistent public interface for plugin management operations.
cmd/plugins/command_test.go (1)
10-160: LGTM! Comprehensive command tests.The test suite properly validates:
- Command structure and subcommands
- List functionality with empty registry
- Enable/disable success paths with state verification
- Error handling for nonexistent plugins
- Proper backup and restoration of global state
cmd/plugins/oadp/oadp.go (3)
7-28: LGTM! Clean plugin interface implementation.The plugin properly implements the required interface with:
- Dependency injection via
ConfigCheckerto avoid import cycles- Defensive nil check in
IsEnabled()- Clear, descriptive return values
30-33: TODO noted for future implementation.The placeholder is appropriate for this foundation PR. As mentioned in the PR description, OADP backup functionality will be moved behind the plugin system in future commits.
35-45: LGTM! Well-designed constructors.The two constructors support both standalone usage (
NewPlugin) and dependency injection (NewPluginWithConfigChecker), providing flexibility for different use cases and testing.cmd/plugins/config.go (5)
32-79: LGTM! Robust configuration persistence.The implementation properly handles:
- Missing config files by returning defaults (lines 37-41)
- Nil map initialization (lines 53-55)
- Directory creation before writing (lines 65-67)
- Appropriate error wrapping throughout
16-91: Note: config cache is not concurrency-safe.The package-level
configvariable (line 16) andGetConfig()(lines 82-91) lack mutex protection. For CLI applications, this is typically acceptable since they're single-threaded. However, if this package is ever used in a concurrent context (e.g., server-side code or parallel test execution), consider adding synchronization.
93-117: LGTM! Safe plugin enablement API.The functions implement appropriate defaults:
IsPluginEnabledreturnsfalseon error or when plugin is not explicitly enabledSetPluginEnabledupdates both the cache and persists to disk- Cache is updated before save to maintain consistency (line 114)
119-122: LGTM! Consistent public API.This wrapper provides a clean public interface for loading configuration.
18-30: No actionable fixes needed; error handling is present but could be clearer.The review comment raised concerns about "silent failure," but verification shows both
loadConfig()andsaveConfig()properly return errors when the config path is empty. However, the error messages ("failed to read config file" / "failed to write config file") mask the root cause—inability to determine the home directory—which could complicate debugging. While the current error handling is acceptable, consider explicitly validating the config path or providing more specific error context.
| func enablePlugin(pluginName string) error { | ||
| // Verify plugin exists | ||
| found := false | ||
| for _, plugin := range GetAllPlugins() { | ||
| if plugin.Name() == pluginName { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| return fmt.Errorf("plugin '%s' not found", pluginName) | ||
| } | ||
|
|
||
| if err := SetPluginEnabled(pluginName, true); err != nil { | ||
| return fmt.Errorf("failed to enable plugin '%s': %w", pluginName, err) | ||
| } | ||
|
|
||
| fmt.Printf("Plugin '%s' enabled successfully\n", pluginName) | ||
| return nil | ||
| } | ||
|
|
||
| func disablePlugin(pluginName string) error { | ||
| // Verify plugin exists | ||
| found := false | ||
| for _, plugin := range GetAllPlugins() { | ||
| if plugin.Name() == pluginName { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| return fmt.Errorf("plugin '%s' not found", pluginName) | ||
| } | ||
|
|
||
| if err := SetPluginEnabled(pluginName, false); err != nil { | ||
| return fmt.Errorf("failed to disable plugin '%s': %w", pluginName, err) | ||
| } | ||
|
|
||
| fmt.Printf("Plugin '%s' disabled successfully\n", pluginName) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate plugin verification logic.
Both enablePlugin and disablePlugin contain identical code for verifying plugin existence (lines 81-92 vs 103-114). This duplication should be extracted into a helper function.
Apply this refactor to reduce duplication:
+func verifyPluginExists(pluginName string) error {
+ for _, plugin := range GetAllPlugins() {
+ if plugin.Name() == pluginName {
+ return nil
+ }
+ }
+ return fmt.Errorf("plugin '%s' not found", pluginName)
+}
+
func enablePlugin(pluginName string) error {
- // Verify plugin exists
- found := false
- for _, plugin := range GetAllPlugins() {
- if plugin.Name() == pluginName {
- found = true
- break
- }
- }
-
- if !found {
- return fmt.Errorf("plugin '%s' not found", pluginName)
+ if err := verifyPluginExists(pluginName); err != nil {
+ return err
}
if err := SetPluginEnabled(pluginName, true); err != nil {
@@ -101,16 +92,8 @@ func enablePlugin(pluginName string) error {
}
func disablePlugin(pluginName string) error {
- // Verify plugin exists
- found := false
- for _, plugin := range GetAllPlugins() {
- if plugin.Name() == pluginName {
- found = true
- break
- }
- }
-
- if !found {
- return fmt.Errorf("plugin '%s' not found", pluginName)
+ if err := verifyPluginExists(pluginName); err != nil {
+ return err
}
if err := SetPluginEnabled(pluginName, false); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func enablePlugin(pluginName string) error { | |
| // Verify plugin exists | |
| found := false | |
| for _, plugin := range GetAllPlugins() { | |
| if plugin.Name() == pluginName { | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| return fmt.Errorf("plugin '%s' not found", pluginName) | |
| } | |
| if err := SetPluginEnabled(pluginName, true); err != nil { | |
| return fmt.Errorf("failed to enable plugin '%s': %w", pluginName, err) | |
| } | |
| fmt.Printf("Plugin '%s' enabled successfully\n", pluginName) | |
| return nil | |
| } | |
| func disablePlugin(pluginName string) error { | |
| // Verify plugin exists | |
| found := false | |
| for _, plugin := range GetAllPlugins() { | |
| if plugin.Name() == pluginName { | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| return fmt.Errorf("plugin '%s' not found", pluginName) | |
| } | |
| if err := SetPluginEnabled(pluginName, false); err != nil { | |
| return fmt.Errorf("failed to disable plugin '%s': %w", pluginName, err) | |
| } | |
| fmt.Printf("Plugin '%s' disabled successfully\n", pluginName) | |
| return nil | |
| } | |
| func verifyPluginExists(pluginName string) error { | |
| for _, plugin := range GetAllPlugins() { | |
| if plugin.Name() == pluginName { | |
| return nil | |
| } | |
| } | |
| return fmt.Errorf("plugin '%s' not found", pluginName) | |
| } | |
| func enablePlugin(pluginName string) error { | |
| if err := verifyPluginExists(pluginName); err != nil { | |
| return err | |
| } | |
| if err := SetPluginEnabled(pluginName, true); err != nil { | |
| return fmt.Errorf("failed to enable plugin '%s': %w", pluginName, err) | |
| } | |
| fmt.Printf("Plugin '%s' enabled successfully\n", pluginName) | |
| return nil | |
| } | |
| func disablePlugin(pluginName string) error { | |
| if err := verifyPluginExists(pluginName); err != nil { | |
| return err | |
| } | |
| if err := SetPluginEnabled(pluginName, false); err != nil { | |
| return fmt.Errorf("failed to disable plugin '%s': %w", pluginName, err) | |
| } | |
| fmt.Printf("Plugin '%s' disabled successfully\n", pluginName) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In cmd/plugins/command.go around lines 80 to 122, the plugin-existence
verification logic duplicated in enablePlugin and disablePlugin should be
extracted into a single helper. Create a small helper (e.g.,
ensurePluginExists(pluginName string) error or pluginExists(pluginName string)
bool) that iterates GetAllPlugins() and either returns nil / true when found or
a formatted error when not; replace the duplicated loops in both enablePlugin
and disablePlugin with calls to that helper and keep the existing error messages
and subsequent SetPluginEnabled calls unchanged.
|
/retest-required |
1 similar comment
|
/retest-required |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
This PR adds a comprehensive plugin system that allows enabling/disabling CLI functionality without affecting CI operations. Plugins are disabled by default and configured via
~/.config/hcp/config.yaml.Features
hypershift plugins enable/disable)hypershift plugins list)Implementation Details
The plugin system provides:
Breaking Changes
Test Plan
Fixes
This implements the foundation for CNTRLPLANE-1691 to allow hiding functionality behind plugins without affecting CI operations.
🤖 Generated with Claude Code