Merge pull request #16 from Zipstack/feature/command-registration

feat: implement command registry and init command

Author: ritwik-g

.gitignore

@@ -122,6 +122,7 @@ ipython_config.py
 __pypackages__/
 
 # Helm Values Manager specific
+helm-values.json
 .helm-values.lock
 
 # Celery stuff

docs/ADRs/008-remove-command-registry.md

@@ -0,0 +1,74 @@
+# 8. Remove Command Registry Pattern
+
+Date: 2025-02-26
+
+## Status
+
+Accepted
+
+## Context
+
+We initially implemented a Command Registry pattern to manage and discover commands in our Helm plugin. The registry was designed to:
+- Register commands with unique names
+- Retrieve command instances by name
+- Potentially support future features like plugins, middleware, and dynamic command loading
+
+However, our current implementation shows that:
+1. Commands are statically defined in CLI functions using Typer
+2. Each CLI function directly knows which command to instantiate
+3. We don't have requirements for:
+   - Plugin system or third-party extensions
+   - Dynamic command loading/discovery
+   - Command aliasing or middleware
+   - Runtime command configuration
+
+The registry adds an unnecessary layer of indirection:
+```python
+# Current approach with registry
+registry = CommandRegistry()
+registry.register("init", InitCommand)
+command = registry.get_command("init")()
+
+# Simplified to
+command = InitCommand()
+```
+
+## Decision
+
+We will remove the Command Registry pattern and use direct command instantiation because:
+1. It simplifies the code by removing unnecessary abstraction
+2. It follows YAGNI (You Ain't Gonna Need It) principle
+3. Our commands are fixed and well-defined as part of the Helm plugin
+4. Typer handles CLI command registration and discovery
+5. We don't have current or planned requirements that would benefit from a registry
+
+## Consequences
+
+### Positive
+1. Simpler, more maintainable code
+2. Less indirection and complexity
+3. Easier to understand command flow
+4. Reduced testing surface
+5. Better alignment with Typer's design
+
+### Negative
+1. If we need these features in the future, we'll need to:
+   - Reimplement the registry pattern
+   - Update all command instantiation points
+   - Add command discovery mechanism
+
+### Neutral
+1. Command implementation and interface remain unchanged
+2. CLI functionality remains the same
+3. User experience is unaffected
+
+## Future Considerations
+
+If we need registry features in the future, we should consider:
+1. Plugin system requirements
+2. Command discovery needs
+3. Middleware requirements
+4. Integration with Helm plugin system
+
+## Related
+- [ADR-001] Initial Architecture Decision

docs/ADRs/README.md

@@ -55,6 +55,12 @@ An Architecture Decision Record (ADR) is a document that captures an important a
 - **Impact**: Ensures security while maintaining flexibility and traceability
 - **Dependencies**: ADR-005
 
+### [ADR-008: Remove Command Registry](008-remove-command-registry.md)
+- **Status**: Accepted
+- **Context**: Command Registry pattern adds unnecessary complexity
+- **Decision**: Remove registry in favor of direct command instantiation
+- **Impact**: Simplifies code and aligns better with Typer's design
+
 ## ADR Template
 For new ADRs, use this template:
 ```markdown

docs/Development/tasks.md

@@ -81,14 +81,14 @@
   - [x] Implement BaseCommand class with basic flow
     - [x] Add configuration loading/saving
     - [x] Add error handling and logging
-  - [ ] Add command registration in CLI
-  - [ ] Add basic command discovery
+  - [x] Add command registration in CLI
+  - [x] Add basic command discovery
 
-- [ ] Configuration Setup Commands
-  - [ ] Implement init command
-    - [ ] Add empty config initialization
-    - [ ] Add config file creation
-    - [ ] Add schema template generation
+- [x] Configuration Setup Commands
+  - [x] Implement init command
+    - [x] Add empty config initialization
+    - [x] Add config file creation
+    - [x] Add schema template generation
   - [ ] Implement add-value-config command
     - [ ] Add basic path validation
     - [ ] Add metadata validation
@@ -110,9 +110,9 @@
     - [ ] Add value storage
 
 #### Phase 2: Enhanced Safety & Management
-- [ ] Enhanced Command Infrastructure
-  - [ ] Add file locking mechanism
-  - [ ] Add atomic writes
+- [x] Enhanced Command Infrastructure
+  - [x] Add file locking mechanism
+  - [x] Add atomic writes
   - [ ] Add basic backup strategy
 
 - [ ] Configuration Management
@@ -148,11 +148,11 @@
   - [ ] Add help text improvements
   - [ ] Add usage examples
 
-- [ ] Testing Infrastructure
-  - [ ] Add command test fixtures
-  - [ ] Add mock file system
-  - [ ] Add mock backend
-  - [ ] Add integration tests
+- [x] Testing Infrastructure
+  - [x] Add command test fixtures
+  - [x] Add mock file system
+  - [x] Add mock backend
+  - [x] Add integration tests
 
 - [ ] Final Touches
   - [ ] Add command output formatting
@@ -170,10 +170,10 @@
   - [x] ConfigMetadata tests
   - [ ] Backend tests
   - [ ] Command tests
-- [ ] Add integration tests
-  - [ ] End-to-end command tests
-  - [ ] Backend integration tests
-  - [ ] File operation tests
+- [x] Add integration tests
+  - [x] End-to-end command tests
+  - [x] Backend integration tests
+  - [x] File operation tests
 
 ### Logging System
 - [x] Implement Helm-style logger

helm_values_manager/cli.py

@@ -2,6 +2,9 @@
 
 import typer
 
+from helm_values_manager.commands.init_command import InitCommand
+from helm_values_manager.utils.logger import HelmLogger
+
 COMMAND_INFO = "helm values-manager"
 
 app = typer.Typer(
@@ -28,16 +31,15 @@ def main(ctx: typer.Context):
 @app.command()
 def init(
     release_name: str = typer.Option(..., "--release", "-r", help="Name of the Helm release"),
-    config_file: str = typer.Option(
-        "values-manager.yaml",
-        "--config",
-        "-c",
-        help="Path to the values manager configuration file",
-    ),
 ):
     """Initialize a new values manager configuration."""
-    typer.echo(f"Initializing values manager with config file: {config_file}, for the release: {release_name}.")
-    # TODO: Implement initialization logic
+    try:
+        command = InitCommand()
+        result = command.execute(release_name=release_name)
+        typer.echo(result)
+    except Exception as e:
+        HelmLogger.error("Failed to initialize: %s", str(e))
+        raise typer.Exit(code=1) from e
 
 
 if __name__ == "__main__":

helm_values_manager/commands/base_command.py

@@ -108,16 +108,19 @@ def release_lock(self) -> None:
             self._lock_fd = None
             HelmLogger.debug("Released lock on file %s", self.lock_file)
 
-    def execute(self) -> Any:
+    def execute(self, **kwargs) -> Any:
         """Execute the command.
 
         This is the main entry point for running a command.
         It handles:
         1. Lock acquisition
-        2. Configuration loading
+        2. Configuration loading (if needed)
         3. Command execution via run()
         4. Lock release
 
+        Args:
+            **kwargs: Command-specific keyword arguments
+
         Returns:
             Any: The result of the command execution.
 
@@ -126,20 +129,23 @@ def execute(self) -> Any:
         """
         try:
             self.acquire_lock()
-            config = self.load_config()
-            result = self.run(config)
+            config = None
+            if not getattr(self, "skip_config_load", False):
+                config = self.load_config()
+            result = self.run(config=config, **kwargs)
             return result
         finally:
             self.release_lock()
 
-    def run(self, config: HelmValuesConfig) -> Any:
+    def run(self, config: Optional[HelmValuesConfig] = None, **kwargs) -> Any:
         """Run the command-specific logic.
 
         This method should be implemented by each specific command subclass
         to perform its unique functionality.
 
         Args:
-            config: The loaded configuration.
+            config: The loaded configuration, or None if skip_config_load is True
+            **kwargs: Command-specific keyword arguments
 
         Returns:
             Any: The result of running the command.

helm_values_manager/commands/init_command.py

@@ -0,0 +1,52 @@
+"""Command to initialize a new helm-values configuration."""
+
+import os
+from typing import Optional
+
+from helm_values_manager.commands.base_command import BaseCommand
+from helm_values_manager.models.helm_values_config import HelmValuesConfig
+from helm_values_manager.utils.logger import HelmLogger
+
+
+class InitCommand(BaseCommand):
+    """Command to initialize a new helm-values configuration."""
+
+    def __init__(self) -> None:
+        """Initialize the init command."""
+        super().__init__()
+        self.skip_config_load = True
+
+    def run(self, config: Optional[HelmValuesConfig] = None, **kwargs) -> str:
+        """
+        Initialize a new helm-values configuration.
+
+        Args:
+            config: Not used by this command
+            **kwargs: Command arguments
+                - release_name (str): Name of the Helm release
+
+        Returns:
+            str: Success message
+
+        Raises:
+            FileExistsError: If configuration file already exists
+            ValueError: If release name is invalid
+        """
+        release_name = kwargs.get("release_name")
+        if not release_name:
+            raise ValueError("Release name cannot be empty")
+
+        # Check if config file already exists
+        if os.path.exists(self.config_file):
+            raise FileExistsError(f"Configuration file {self.config_file} already exists")
+
+        # Create new config
+        new_config = HelmValuesConfig()
+        new_config.version = "1.0"
+        new_config.release = release_name
+
+        # Save config
+        self.save_config(new_config)
+
+        HelmLogger.debug("Initialized helm-values configuration for release: %s", release_name)
+        return "Successfully initialized helm-values configuration."

helm_values_manager/models/helm_values_config.py

@@ -131,8 +131,16 @@ def set_value(self, path: str, environment: str, value: str) -> None:
         self._path_map[path].set_value(environment, value_obj)
 
     def validate(self) -> None:
-        """Validate the configuration."""
-        # Validate against JSON schema first
+        """
+        Validate the configuration.
+
+        Raises:
+            ValueError: If validation fails.
+        """
+        if not self.release:
+            raise ValueError("Release name is required")
+
+        # Validate schema
         self._validate_schema(self.to_dict())
 
         # Then validate each path_data

tests/integration/test_cli_integration.py

@@ -1,5 +1,6 @@
 """Integration tests for the helm-values-manager CLI."""
 
+import os
 import subprocess
 from pathlib import Path
 
@@ -70,13 +71,17 @@ def test_init_help_command(plugin_install):
 
     assert returncode == 0
     assert "Initialize a new values manager configuration" in stdout
-    assert "--config" in stdout
-    assert "-c" in stdout
+    assert "--release" in stdout
+    assert "-r" in stdout
 
 
 def test_init_command(plugin_install, tmp_path):
-    """Test that the init command works with default config file."""
+    """Test that the init command works."""
+    # Change to temp directory to avoid conflicts
+    os.chdir(tmp_path)
     stdout, stderr, returncode = run_helm_command(["values-manager", "init", "-r", "test"])
 
     assert returncode == 0
-    assert "Initializing values manager with config file: values-manager.yaml, for the release: test." in stdout
+    assert "Successfully initialized helm-values configuration" in stdout
+    assert Path("helm-values.json").exists()
+    assert Path(".helm-values.lock").exists()