feat: add config validation and improve documentation

docs/Design/sequence-diagrams.md

@@ -661,19 +661,19 @@ sequenceDiagram
 Each diagram shows:
 - The exact CLI command being executed
 - All components involved in processing the command
-- Data flow between components
-- Validation steps
-- File system operations
-- Success/error handling
-
-The main CLI commands covered are:
-1. `init` - Initialize new configuration
-2. `add-value-config` - Define a new value configuration with metadata
+- The sequence of operations and data flow
+- Error handling and validation steps
+- Lock management for concurrent access
+
+## Commands Covered
+
+1. `init` - Initialize a new helm-values configuration
+2. `add-value-config` - Add a new value configuration with metadata
 3. `add-deployment` - Add a new deployment configuration
-4. `set-value` - Set a value for a specific path and environment
-5. `get-value` - Retrieve a value for a specific path and environment
-6. `validate` - Validate the entire configuration
-7. `generate` - Generate values.yaml for a specific environment
+4. `add-backend` - Add a backend to a deployment
+5. `add-auth` - Add authentication to a deployment
+6. `get-value` - Get a value for a specific path and environment
+7. `set-value` - Set a value for a specific path and environment
 8. `list-values` - List all values for a specific environment
 9. `list-deployments` - List all deployments
 10. `remove-deployment` - Remove a deployment configuration

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)
 

helm_values_manager/schemas/v1.json

@@ -10,11 +10,17 @@
         },
         "release": {
             "type": "string",
+            "pattern": "^[a-z0-9][a-z0-9-]*[a-z0-9]$",
+            "maxLength": 53,
             "description": "Name of the Helm release"
         },
         "deployments": {
             "type": "object",
             "description": "Map of deployment names to their configurations",
+            "propertyNames": {
+                "pattern": "^[a-z0-9][a-z0-9-]*[a-z0-9]$",
+                "description": "Deployment names must be lowercase alphanumeric with hyphens, cannot start/end with hyphen"
+            },
             "additionalProperties": {
                 "type": "object",
                 "required": ["backend", "auth"],

helm_values_manager/utils/logger.py

@@ -15,7 +15,7 @@ class HelmLogger:
     Logger class that follows Helm plugin conventions.
 
     This logger:
-    1. Writes debug messages only when HELM_DEBUG is set
+    1. Writes debug messages only when HELM_DEBUG is set and not "0" or "false"
     2. Writes all messages to stderr (Helm convention)
     3. Uses string formatting for better performance
     4. Provides consistent error and debug message formatting
@@ -24,13 +24,14 @@ class HelmLogger:
     @staticmethod
     def debug(msg: str, *args: Any) -> None:
         """
-        Print debug message if HELM_DEBUG is set.
+        Print debug message if HELM_DEBUG is set and not "0" or "false".
 
         Args:
             msg: Message with optional string format placeholders
             args: Values to substitute in the message
         """
-        if os.environ.get("HELM_DEBUG"):
+        debug_val = os.environ.get("HELM_DEBUG", "").lower()
+        if debug_val and debug_val not in ("0", "false"):
             if args:
                 msg = msg % args
             print("[debug] %s" % msg, file=sys.stderr)

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"