Merge pull request #27 from Zipstack/feature/validate-config-before-save

feat: add config validation and improve documentation

Author: ritwik-g

docs/Design/low-level-design.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

docs/Design/sequence-diagrams.md

@@ -1,13 +1,12 @@
 """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
 from helm_values_manager.commands.add_value_config_command import AddValueConfigCommand
 from helm_values_manager.commands.init_command import InitCommand
 from helm_values_manager.commands.set_value_command import SetValueCommand
+from helm_values_manager.models.config_metadata import ConfigMetadata
 from helm_values_manager.utils.logger import HelmLogger
 
 COMMAND_INFO = "helm values-manager"
@@ -50,12 +49,14 @@ 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(
+        ConfigMetadata.DEFAULT_DESCRIPTION, "--description", "-d", help="Description of what this configuration does"
+    ),
+    required: bool = typer.Option(
+        ConfigMetadata.DEFAULT_REQUIRED, "--required", "-r", help="Whether this configuration is required"
     ),
-    required: bool = typer.Option(False, "--required", "-r", help="Whether this configuration is required"),
     sensitive: bool = typer.Option(
-        False,
+        ConfigMetadata.DEFAULT_SENSITIVE,
         "--sensitive",
         "-s",
         help="Whether this configuration contains sensitive data (coming in v0.2.0)",

helm_values_manager/cli.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/add_value_config_command.py

@@ -73,7 +73,12 @@ def save_config(self, config: HelmValuesConfig) -> None:
 
         Raises:
             IOError: If unable to write to the file.
+            ValueError: If the configuration is invalid (e.g., missing release name).
+            ValidationError: If JSON schema validation fails.
         """
+        # 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/commands/base_command.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/config_metadata.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.
@@ -135,7 +140,8 @@ def validate(self) -> None:
         Validate the configuration.
 
         Raises:
-            ValueError: If validation fails.
+            ValueError: If validation fails (e.g., missing release name)
+            ValidationError: If JSON schema validation fails
         """
         if not self.release:
             raise ValueError("Release name is required")
@@ -191,9 +197,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/helm_values_config.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: Optional[Dict[str, Any]] = None):
         """
         Initialize PathData with a path and metadata.
 
@@ -24,10 +24,12 @@ def __init__(self, path: str, metadata: Dict[str, Any]):
             metadata: Dictionary containing metadata for the path
         """
         self.path = path
+        if metadata is None:
+            metadata = {}
         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 +162,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/models/path_data.py

@@ -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/schemas/v1.json

@@ -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,16 +24,18 @@ 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"):
-            if args:
-                msg = msg % args
-            print("[debug] %s" % msg, file=sys.stderr)
+        debug_val = os.environ.get("HELM_DEBUG", "false").lower()
+        if debug_val in ("0", "false"):
+            return
+        if args:
+            msg = msg % args
+        print("[debug] %s" % msg, file=sys.stderr)
 
     @staticmethod
     def error(msg: str, *args: Any) -> None:

helm_values_manager/utils/logger.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)