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: