feat: validate config before saving to disk

- Added validation call in save_config to ensure config is valid before saving
- Updated tests to verify validation behavior
- Added test for empty description handling
- Added test for schema validation errors

Author: ritwik-g

helm_values_manager/cli.py

@@ -1,7 +1,5 @@
 """Command line interface for the helm-values-manager plugin."""
 
-from typing import Optional
-
 import typer
 
 from helm_values_manager.commands.add_deployment_command import AddDeploymentCommand
@@ -50,8 +48,8 @@ def init(
 @app.command("add-value-config")
 def add_value_config(
     path: str = typer.Option(..., "--path", "-p", help="Configuration path (e.g., 'app.replicas')"),
-    description: Optional[str] = typer.Option(
-        None, "--description", "-d", help="Description of what this configuration does"
+    description: str = typer.Option(
+        "Description of the configuration", "--description", "-d", help="Description of what this configuration does"
     ),
     required: bool = typer.Option(False, "--required", "-r", help="Whether this configuration is required"),
     sensitive: bool = typer.Option(

helm_values_manager/commands/add_value_config_command.py

@@ -3,6 +3,7 @@
 from typing import Optional
 
 from helm_values_manager.commands.base_command import BaseCommand
+from helm_values_manager.models.config_metadata import ConfigMetadata
 from helm_values_manager.models.helm_values_config import HelmValuesConfig
 from helm_values_manager.utils.logger import HelmLogger
 
@@ -35,9 +36,9 @@ def run(self, config: Optional[HelmValuesConfig] = None, **kwargs) -> str:
         if not path:
             raise ValueError("Path cannot be empty")
 
-        description = kwargs.get("description")
-        required = kwargs.get("required", False)
-        sensitive = kwargs.get("sensitive", False)
+        description = kwargs.get("description", ConfigMetadata.DEFAULT_DESCRIPTION)
+        required = kwargs.get("required", ConfigMetadata.DEFAULT_REQUIRED)
+        sensitive = kwargs.get("sensitive", ConfigMetadata.DEFAULT_SENSITIVE)
 
         try:
             # Add the new configuration path

helm_values_manager/commands/base_command.py

@@ -73,7 +73,11 @@ def save_config(self, config: HelmValuesConfig) -> None:
 
         Raises:
             IOError: If unable to write to the file.
+            ValueError: If the configuration is invalid.
         """
+        # Validate the config before saving
+        config.validate()
+
         try:
             with open(self.config_file, "w", encoding="utf-8") as f:
                 json.dump(config.to_dict(), f, indent=2)

helm_values_manager/models/config_metadata.py

@@ -1,7 +1,7 @@
 """Simple dataclass for configuration metadata."""
 
 from dataclasses import asdict, dataclass
-from typing import Any, Dict, Optional
+from typing import Any, ClassVar, Dict
 
 
 @dataclass
@@ -10,14 +10,19 @@ class ConfigMetadata:
     Represents metadata for a configuration path.
 
     Attributes:
-        description (Optional[str]): Description of the configuration path.
+        description (str): Description of the configuration path. Defaults to empty string.
         required (bool): Whether the configuration path is required. Defaults to False.
         sensitive (bool): Whether the configuration path is sensitive. Defaults to False.
     """
 
-    description: Optional[str] = None
-    required: bool = False
-    sensitive: bool = False
+    # Default values as class variables for reference elsewhere
+    DEFAULT_DESCRIPTION: ClassVar[str] = ""
+    DEFAULT_REQUIRED: ClassVar[bool] = False
+    DEFAULT_SENSITIVE: ClassVar[bool] = False
+
+    description: str = DEFAULT_DESCRIPTION
+    required: bool = DEFAULT_REQUIRED
+    sensitive: bool = DEFAULT_SENSITIVE
 
     def to_dict(self) -> Dict[str, Any]:
         """Convert metadata to dictionary."""
@@ -27,7 +32,7 @@ def to_dict(self) -> Dict[str, Any]:
     def from_dict(cls, data: Dict[str, Any]) -> "ConfigMetadata":
         """Create a ConfigMetadata instance from a dictionary."""
         return cls(
-            description=data.get("description"),
-            required=data.get("required", False),
-            sensitive=data.get("sensitive", False),
+            description=data.get("description", cls.DEFAULT_DESCRIPTION),
+            required=data.get("required", cls.DEFAULT_REQUIRED),
+            sensitive=data.get("sensitive", cls.DEFAULT_SENSITIVE),
         )

helm_values_manager/models/helm_values_config.py

@@ -9,6 +9,7 @@
 from jsonschema.exceptions import ValidationError
 
 from helm_values_manager.backends.simple import SimpleValueBackend
+from helm_values_manager.models.config_metadata import ConfigMetadata
 from helm_values_manager.models.path_data import PathData
 from helm_values_manager.models.value import Value
 from helm_values_manager.utils.logger import HelmLogger
@@ -65,7 +66,11 @@ def _validate_schema(cls, data: dict) -> None:
             raise
 
     def add_config_path(
-        self, path: str, description: Optional[str] = None, required: bool = False, sensitive: bool = False
+        self,
+        path: str,
+        description: str = ConfigMetadata.DEFAULT_DESCRIPTION,
+        required: bool = ConfigMetadata.DEFAULT_REQUIRED,
+        sensitive: bool = ConfigMetadata.DEFAULT_SENSITIVE,
     ) -> None:
         """
         Add a new configuration path.
@@ -191,9 +196,9 @@ def from_dict(cls, data: dict) -> "HelmValuesConfig":
         for config_item in data.get("config", []):
             path = config_item["path"]
             metadata = {
-                "description": config_item.get("description"),
-                "required": config_item.get("required", False),
-                "sensitive": config_item.get("sensitive", False),
+                "description": config_item.get("description", ConfigMetadata.DEFAULT_DESCRIPTION),
+                "required": config_item.get("required", ConfigMetadata.DEFAULT_REQUIRED),
+                "sensitive": config_item.get("sensitive", ConfigMetadata.DEFAULT_SENSITIVE),
             }
             path_data = PathData(path, metadata)
             config._path_map[path] = path_data

helm_values_manager/models/path_data.py

@@ -15,7 +15,7 @@
 class PathData:
     """Manages metadata and values for a configuration path."""
 
-    def __init__(self, path: str, metadata: Dict[str, Any]):
+    def __init__(self, path: str, metadata: Dict[str, Any] = {}):
         """
         Initialize PathData with a path and metadata.
 
@@ -25,9 +25,9 @@ def __init__(self, path: str, metadata: Dict[str, Any]):
         """
         self.path = path
         self._metadata = ConfigMetadata(
-            description=metadata.get("description"),
-            required=metadata.get("required", False),
-            sensitive=metadata.get("sensitive", False),
+            description=metadata.get("description", ConfigMetadata.DEFAULT_DESCRIPTION),
+            required=metadata.get("required", ConfigMetadata.DEFAULT_REQUIRED),
+            sensitive=metadata.get("sensitive", ConfigMetadata.DEFAULT_SENSITIVE),
         )
         self._values: Dict[str, Value] = {}
         HelmLogger.debug("Created PathData instance for path %s", path)
@@ -160,9 +160,9 @@ def from_dict(
             raise ValueError(f"Missing required keys: {missing}")
 
         metadata = {
-            "description": data.get("description"),
-            "required": data.get("required", False),
-            "sensitive": data.get("sensitive", False),
+            "description": data.get("description", ConfigMetadata.DEFAULT_DESCRIPTION),
+            "required": data.get("required", ConfigMetadata.DEFAULT_REQUIRED),
+            "sensitive": data.get("sensitive", ConfigMetadata.DEFAULT_SENSITIVE),
         }
         path_data = cls(path=data["path"], metadata=metadata)
 

tests/unit/commands/test_base_command.py

@@ -159,29 +159,89 @@ def test_execute_ensures_lock_release_on_error(base_command, valid_config):
 
 
 def test_save_config_success(base_command):
-    """Test successful config saving."""
+    """Test successful config save."""
     config = HelmValuesConfig()
     config.version = "1.0"
     config.release = "test"
 
+    # Mock the validate method
+    config.validate = MagicMock()
+
     with patch("builtins.open", mock_open()) as mock_file:
         base_command.save_config(config)
 
+        # Verify validate was called
+        config.validate.assert_called_once()
+
         mock_file.assert_called_once_with(base_command.config_file, "w", encoding="utf-8")
         handle = mock_file()
 
+        # Verify the written data
         written_json = "".join(call.args[0] for call in handle.write.call_args_list)
         written_data = json.loads(written_json)
-        assert written_data["version"] == config.version
-        assert written_data["release"] == config.release
+        assert written_data["version"] == "1.0"
+        assert written_data["release"] == "test"
 
 
 def test_save_config_io_error(base_command):
-    """Test save_config when IO error occurs."""
+    """Test IO error handling when saving config."""
     config = HelmValuesConfig()
-    error_message = "Test error"
+    config.version = "1.0"
+    config.release = "test"
 
-    with patch("builtins.open", mock_open()) as mock_file:
-        mock_file.return_value.write.side_effect = IOError(error_message)
-        with pytest.raises(IOError, match=error_message):
+    # Mock the validate method
+    config.validate = MagicMock()
+
+    # Simulate an IO error
+    mock_file = mock_open()
+    mock_file.side_effect = IOError("Test IO Error")
+
+    with patch("builtins.open", mock_file):
+        with pytest.raises(IOError) as excinfo:
             base_command.save_config(config)
+
+        assert "Test IO Error" in str(excinfo.value)
+
+        # Verify validate was called
+        config.validate.assert_called_once()
+
+
+def test_save_config_validates_schema(base_command):
+    """Test that save_config validates the schema before saving."""
+    # Create a mock config
+    config = MagicMock(spec=HelmValuesConfig)
+
+    # Make validate method raise an error
+    config.validate.side_effect = ValueError("Schema validation failed")
+
+    # Try to save the config
+    with pytest.raises(ValueError, match="Schema validation failed"):
+        base_command.save_config(config)
+
+    # Verify that validate was called
+    config.validate.assert_called_once()
+
+
+def test_save_config_with_empty_description(base_command, tmp_path):
+    """Test that save_config handles empty description correctly."""
+    # Create a real config
+    config = HelmValuesConfig()
+    config.release = "test"
+
+    # Add a config path with empty description (default)
+    config.add_config_path("test.path")
+
+    # Set a temporary config file
+    temp_file = tmp_path / "test_config.json"
+    base_command.config_file = str(temp_file)
+
+    # Save the config
+    base_command.save_config(config)
+
+    # Read the saved file
+    with open(temp_file, "r") as f:
+        data = json.load(f)
+
+    # Verify that description is an empty string
+    assert data["config"][0]["description"] == ""
+    assert isinstance(data["config"][0]["description"], str)

tests/unit/commands/test_init_command.py

@@ -36,18 +36,20 @@ def test_initialization(init_command, tmp_path):
 
 def test_run_creates_config(init_command):
     """Test that run creates a new config with release name."""
-    with patch("builtins.open", mock_open()) as mock_file:
-        result = init_command.run(release_name="test-release")
-        assert result == "Successfully initialized helm-values configuration."
-
-        # Verify config was saved with correct data
-        handle = mock_file()
-        written_json = "".join(call.args[0] for call in handle.write.call_args_list)
-        written_data = json.loads(written_json)
-        assert written_data["version"] == "1.0"
-        assert written_data["release"] == "test-release"
-        assert written_data["deployments"] == {}
-        assert written_data["config"] == []
+    # Mock the validate method
+    with patch("helm_values_manager.models.helm_values_config.HelmValuesConfig.validate"):
+        with patch("builtins.open", mock_open()) as mock_file:
+            result = init_command.run(release_name="test-release")
+            assert result == "Successfully initialized helm-values configuration."
+
+            # Verify config was saved with correct data
+            handle = mock_file()
+            written_json = "".join(call.args[0] for call in handle.write.call_args_list)
+            written_data = json.loads(written_json)
+            assert written_data["version"] == "1.0"
+            assert written_data["release"] == "test-release"
+            assert written_data["deployments"] == {}
+            assert written_data["config"] == []
 
 
 def test_run_with_existing_config(init_command):

tests/unit/commands/test_set_value_command.py

@@ -1,7 +1,7 @@
 """Tests for the set-value command."""
 
 import json
-from unittest.mock import mock_open, patch
+from unittest.mock import MagicMock, mock_open, patch
 
 import pytest
 
@@ -133,3 +133,16 @@ def test_set_value_none_config(command):
     with patch.object(command, "load_config", return_value=None):
         with pytest.raises(ValueError, match="Configuration not loaded"):
             command.execute(path="app.replicas", environment="dev", value="3")
+
+
+def test_set_value_general_error():
+    """Test that general errors are properly handled."""
+    command = SetValueCommand()
+    config = MagicMock()
+    config.deployments = {"dev": MagicMock()}  # Mock deployment exists
+    config.set_value.side_effect = Exception("Unexpected error")
+
+    with pytest.raises(Exception) as exc_info:
+        command.run(config, path="app.replicas", environment="dev", value="3")
+
+    assert str(exc_info.value) == "Unexpected error"

tests/unit/models/test_schema_validation.py

@@ -72,7 +72,7 @@ def test_default_values():
     }
     config = HelmValuesConfig.from_dict(config_data)
     path_data = config._path_map["app.config.key1"]
-    assert path_data.metadata.description is None
+    assert path_data.metadata.description == ""
     assert path_data.metadata.required is False
     assert path_data.metadata.sensitive is False