Skip to content

Commit 8b8b818

Browse files
committed
refactor: improve command registration and test isolation
1. Move command registry initialization into init function to fix test interference 2. Use tmp_path for better test isolation 3. Add proper validation for release name 4. Improve error handling and logging 5. Update tests to verify file creation and content 6. Update tasks.md to mark completed tasks 7. Fix linting issues: - Remove trailing whitespace - Fix docstring formatting - Remove unused imports This change improves test reliability by: - Moving command registration into init function to avoid test interference - Using tmp_path for proper test isolation - Adding better assertions for file operations - Improving error handling and logging
1 parent 436e043 commit 8b8b818

File tree

11 files changed

+222
-39
lines changed

11 files changed

+222
-39
lines changed

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ ipython_config.py
122122
__pypackages__/
123123

124124
# Helm Values Manager specific
125+
helm-values.json
125126
.helm-values.lock
126127

127128
# Celery stuff

docs/Development/tasks.md

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -81,14 +81,14 @@
8181
- [x] Implement BaseCommand class with basic flow
8282
- [x] Add configuration loading/saving
8383
- [x] Add error handling and logging
84-
- [ ] Add command registration in CLI
85-
- [ ] Add basic command discovery
84+
- [x] Add command registration in CLI
85+
- [x] Add basic command discovery
8686

87-
- [ ] Configuration Setup Commands
88-
- [ ] Implement init command
89-
- [ ] Add empty config initialization
90-
- [ ] Add config file creation
91-
- [ ] Add schema template generation
87+
- [x] Configuration Setup Commands
88+
- [x] Implement init command
89+
- [x] Add empty config initialization
90+
- [x] Add config file creation
91+
- [x] Add schema template generation
9292
- [ ] Implement add-value-config command
9393
- [ ] Add basic path validation
9494
- [ ] Add metadata validation
@@ -110,9 +110,9 @@
110110
- [ ] Add value storage
111111

112112
#### Phase 2: Enhanced Safety & Management
113-
- [ ] Enhanced Command Infrastructure
114-
- [ ] Add file locking mechanism
115-
- [ ] Add atomic writes
113+
- [x] Enhanced Command Infrastructure
114+
- [x] Add file locking mechanism
115+
- [x] Add atomic writes
116116
- [ ] Add basic backup strategy
117117

118118
- [ ] Configuration Management
@@ -148,11 +148,11 @@
148148
- [ ] Add help text improvements
149149
- [ ] Add usage examples
150150

151-
- [ ] Testing Infrastructure
152-
- [ ] Add command test fixtures
153-
- [ ] Add mock file system
154-
- [ ] Add mock backend
155-
- [ ] Add integration tests
151+
- [x] Testing Infrastructure
152+
- [x] Add command test fixtures
153+
- [x] Add mock file system
154+
- [x] Add mock backend
155+
- [x] Add integration tests
156156

157157
- [ ] Final Touches
158158
- [ ] Add command output formatting
@@ -170,10 +170,10 @@
170170
- [x] ConfigMetadata tests
171171
- [ ] Backend tests
172172
- [ ] Command tests
173-
- [ ] Add integration tests
174-
- [ ] End-to-end command tests
175-
- [ ] Backend integration tests
176-
- [ ] File operation tests
173+
- [x] Add integration tests
174+
- [x] End-to-end command tests
175+
- [x] Backend integration tests
176+
- [x] File operation tests
177177

178178
### Logging System
179179
- [x] Implement Helm-style logger

helm_values_manager/cli.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
import typer
44

5+
from helm_values_manager.commands.init_command import InitCommand
6+
from helm_values_manager.commands.registry import CommandRegistry
7+
from helm_values_manager.utils.logger import HelmLogger
8+
59
COMMAND_INFO = "helm values-manager"
610

711
app = typer.Typer(
@@ -30,8 +34,16 @@ def init(
3034
release_name: str = typer.Option(..., "--release", "-r", help="Name of the Helm release"),
3135
):
3236
"""Initialize a new values manager configuration."""
33-
typer.echo(f"Initializing values manager for the release: {release_name}.")
34-
# TODO: Implement initialization logic
37+
try:
38+
# Initialize command registry and register commands
39+
registry = CommandRegistry()
40+
registry.register("init", InitCommand)
41+
command = registry.get_command("init")()
42+
result = command.execute(release_name=release_name)
43+
typer.echo(result)
44+
except Exception as e:
45+
HelmLogger.error("Failed to initialize: %s", str(e))
46+
raise typer.Exit(code=1) from e
3547

3648

3749
if __name__ == "__main__":

helm_values_manager/commands/base_command.py

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,19 @@ def release_lock(self) -> None:
108108
self._lock_fd = None
109109
HelmLogger.debug("Released lock on file %s", self.lock_file)
110110

111-
def execute(self) -> Any:
111+
def execute(self, **kwargs) -> Any:
112112
"""Execute the command.
113113
114114
This is the main entry point for running a command.
115115
It handles:
116116
1. Lock acquisition
117-
2. Configuration loading
117+
2. Configuration loading (if needed)
118118
3. Command execution via run()
119119
4. Lock release
120120
121+
Args:
122+
**kwargs: Command-specific keyword arguments
123+
121124
Returns:
122125
Any: The result of the command execution.
123126
@@ -126,20 +129,23 @@ def execute(self) -> Any:
126129
"""
127130
try:
128131
self.acquire_lock()
129-
config = self.load_config()
130-
result = self.run(config)
132+
config = None
133+
if not getattr(self, "skip_config_load", False):
134+
config = self.load_config()
135+
result = self.run(config=config, **kwargs)
131136
return result
132137
finally:
133138
self.release_lock()
134139

135-
def run(self, config: HelmValuesConfig) -> Any:
140+
def run(self, config: Optional[HelmValuesConfig] = None, **kwargs) -> Any:
136141
"""Run the command-specific logic.
137142
138143
This method should be implemented by each specific command subclass
139144
to perform its unique functionality.
140145
141146
Args:
142-
config: The loaded configuration.
147+
config: The loaded configuration, or None if skip_config_load is True
148+
**kwargs: Command-specific keyword arguments
143149
144150
Returns:
145151
Any: The result of running the command.
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
"""Command to initialize a new helm-values configuration."""
2+
3+
import os
4+
5+
from helm_values_manager.commands.base_command import BaseCommand
6+
from helm_values_manager.models.helm_values_config import HelmValuesConfig
7+
from helm_values_manager.utils.logger import HelmLogger
8+
9+
10+
class InitCommand(BaseCommand):
11+
"""Command to initialize a new helm-values configuration."""
12+
13+
def __init__(self) -> None:
14+
"""Initialize the init command."""
15+
super().__init__()
16+
self.skip_config_load = True
17+
18+
def run(self, config: None = None, **kwargs) -> str:
19+
"""
20+
Initialize a new helm-values configuration.
21+
22+
Args:
23+
config: Not used by this command
24+
**kwargs: Command arguments
25+
- release_name (str): Name of the Helm release
26+
27+
Returns:
28+
str: Success message
29+
30+
Raises:
31+
FileExistsError: If configuration file already exists
32+
ValueError: If release name is invalid
33+
"""
34+
release_name = kwargs.get("release_name")
35+
if not release_name:
36+
raise ValueError("Release name cannot be empty")
37+
38+
# Check if config file already exists
39+
if os.path.exists(self.config_file):
40+
raise FileExistsError(f"Configuration file {self.config_file} already exists")
41+
42+
# Create new config
43+
config = HelmValuesConfig()
44+
config.version = "1.0"
45+
config.release = release_name
46+
47+
# Save config
48+
self.save_config(config)
49+
50+
HelmLogger.debug("Initialized helm-values configuration for release: %s", release_name)
51+
return "Successfully initialized helm-values configuration."

helm_values_manager/models/helm_values_config.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,16 @@ def set_value(self, path: str, environment: str, value: str) -> None:
131131
self._path_map[path].set_value(environment, value_obj)
132132

133133
def validate(self) -> None:
134-
"""Validate the configuration."""
135-
# Validate against JSON schema first
134+
"""
135+
Validate the configuration.
136+
137+
Raises:
138+
ValueError: If validation fails.
139+
"""
140+
if not self.release:
141+
raise ValueError("Release name is required")
142+
143+
# Validate schema
136144
self._validate_schema(self.to_dict())
137145

138146
# Then validate each path_data

tests/integration/test_cli_integration.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""Integration tests for the helm-values-manager CLI."""
22

3+
import os
34
import subprocess
45
from pathlib import Path
56

@@ -76,7 +77,11 @@ def test_init_help_command(plugin_install):
7677

7778
def test_init_command(plugin_install, tmp_path):
7879
"""Test that the init command works."""
80+
# Change to temp directory to avoid conflicts
81+
os.chdir(tmp_path)
7982
stdout, stderr, returncode = run_helm_command(["values-manager", "init", "-r", "test"])
8083

8184
assert returncode == 0
82-
assert "Initializing values manager for the release: test." in stdout
85+
assert "Successfully initialized helm-values configuration" in stdout
86+
assert Path("helm-values.json").exists()
87+
assert Path(".helm-values.lock").exists()

tests/unit/commands/test_base_command.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,12 @@ def test_execute_success(base_command, valid_config):
136136
patch("helm_values_manager.commands.base_command.fcntl"),
137137
):
138138
result = base_command.execute()
139-
140-
assert isinstance(base_command.run.call_args[0][0], HelmValuesConfig)
141139
assert result == expected_result
140+
# Check that run was called with a config object and empty kwargs
141+
assert base_command.run.call_count == 1
142+
call_args = base_command.run.call_args
143+
assert isinstance(call_args.kwargs["config"], HelmValuesConfig)
144+
assert len(call_args.args) == 0
142145

143146

144147
def test_execute_ensures_lock_release_on_error(base_command, valid_config):
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
"""Unit tests for the init command."""
2+
3+
import json
4+
from unittest.mock import MagicMock, mock_open, patch
5+
6+
import pytest
7+
8+
from helm_values_manager.commands.init_command import InitCommand
9+
from helm_values_manager.models.helm_values_config import HelmValuesConfig
10+
11+
12+
@pytest.fixture
13+
def init_command(tmp_path):
14+
"""Fixture for InitCommand instance."""
15+
command = InitCommand()
16+
command.config_file = str(tmp_path / "helm-values.json")
17+
command.lock_file = str(tmp_path / ".helm-values.lock")
18+
return command
19+
20+
21+
@pytest.fixture
22+
def mock_config():
23+
"""Fixture for mocked HelmValuesConfig."""
24+
config = MagicMock(spec=HelmValuesConfig)
25+
config.release = "test-release"
26+
config.version = "1.0"
27+
return config
28+
29+
30+
def test_initialization(init_command, tmp_path):
31+
"""Test InitCommand initialization."""
32+
assert isinstance(init_command, InitCommand)
33+
assert init_command.config_file == str(tmp_path / "helm-values.json")
34+
assert init_command.lock_file == str(tmp_path / ".helm-values.lock")
35+
36+
37+
def test_run_creates_config(init_command):
38+
"""Test that run creates a new config with release name."""
39+
with patch("builtins.open", mock_open()) as mock_file:
40+
result = init_command.run(release_name="test-release")
41+
assert result == "Successfully initialized helm-values configuration."
42+
43+
# Verify config was saved with correct data
44+
handle = mock_file()
45+
written_json = "".join(call.args[0] for call in handle.write.call_args_list)
46+
written_data = json.loads(written_json)
47+
assert written_data["version"] == "1.0"
48+
assert written_data["release"] == "test-release"
49+
assert written_data["deployments"] == {}
50+
assert written_data["config"] == []
51+
52+
53+
def test_run_with_existing_config(init_command):
54+
"""Test that run fails if config file already exists."""
55+
with patch("os.path.exists", return_value=True):
56+
with pytest.raises(FileExistsError, match="Configuration file .* already exists"):
57+
init_command.run(release_name="test-release")
58+
59+
60+
def test_run_with_invalid_release_name(init_command):
61+
"""Test that run validates release name."""
62+
with pytest.raises(ValueError, match="Release name cannot be empty"):
63+
init_command.run(release_name="")
64+
65+
with pytest.raises(ValueError, match="Release name cannot be empty"):
66+
init_command.run(release_name=None)

tests/unit/models/test_helm_values_config.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ def test_to_dict_from_dict():
165165
def test_validate():
166166
"""Test validation of required paths."""
167167
config = HelmValuesConfig()
168+
config.release = "test-release" # Set release name
168169
path = "app.config.key1"
169170
config.add_config_path(path, description="Test config", required=True, sensitive=False)
170171

@@ -176,6 +177,7 @@ def test_validate_with_schema():
176177
"""Test validation including schema validation."""
177178
# Create config with invalid version
178179
config = HelmValuesConfig()
180+
config.release = "test-release" # Set release name
179181
config.version = "invalid" # Version must match pattern in schema
180182

181183
# Should raise ValidationError due to schema validation
@@ -185,3 +187,10 @@ def test_validate_with_schema():
185187
# Fix version and test valid case
186188
config.version = "1.0"
187189
config.validate() # Should not raise error
190+
191+
192+
def test_validate_without_release():
193+
"""Test validation fails when release name is not set."""
194+
config = HelmValuesConfig()
195+
with pytest.raises(ValueError, match="Release name is required"):
196+
config.validate()

0 commit comments

Comments
 (0)