Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ ipython_config.py
__pypackages__/

# Helm Values Manager specific
helm-values.json
.helm-values.lock

# Celery stuff
Expand Down
74 changes: 74 additions & 0 deletions docs/ADRs/008-remove-command-registry.md
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions docs/ADRs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
38 changes: 19 additions & 19 deletions docs/Development/tasks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
18 changes: 10 additions & 8 deletions helm_values_manager/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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__":
Expand Down
18 changes: 12 additions & 6 deletions helm_values_manager/commands/base_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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.
Expand Down
52 changes: 52 additions & 0 deletions helm_values_manager/commands/init_command.py
Original file line number Diff line number Diff line change
@@ -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."
12 changes: 10 additions & 2 deletions helm_values_manager/models/helm_values_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 9 additions & 4 deletions tests/integration/test_cli_integration.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Integration tests for the helm-values-manager CLI."""

import os
import subprocess
from pathlib import Path

Expand Down Expand Up @@ -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()
Loading