diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml new file mode 100644 index 0000000..dee37c3 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -0,0 +1,91 @@ +name: 🐛 Bug Report +description: File a bug report +title: "[Bug]: " +labels: ["bug"] +body: + - type: markdown + attributes: + value: | + Thanks for taking the time to fill out this bug report! + + - type: textarea + id: bug-description + attributes: + label: Bug Description + description: A clear and concise description of what the bug is + placeholder: When I run X command, Y happens instead of Z... + validations: + required: true + + - type: textarea + id: reproduction + attributes: + label: Steps To Reproduce + description: Steps to reproduce the behavior + placeholder: | + 1. Run command '...' + 2. With input '...' + 3. See error + validations: + required: true + + - type: textarea + id: expected + attributes: + label: Expected Behavior + description: What did you expect to happen? + placeholder: The command should have... + validations: + required: true + + - type: dropdown + id: os + attributes: + label: Operating System + description: What OS are you using? + options: + - macOS + - Linux + - Windows + validations: + required: true + + - type: input + id: python-version + attributes: + label: Python Version + description: What version of Python are you using? + placeholder: e.g., 3.9.6 + validations: + required: true + + - type: input + id: helm-version + attributes: + label: Helm Version + description: What version of Helm are you using? + placeholder: e.g., 3.12.0 + validations: + required: true + + - type: input + id: package-version + attributes: + label: Package Version + description: What version of helm-values-manager are you using? + placeholder: e.g., 0.1.0 + validations: + required: true + + - type: textarea + id: logs + attributes: + label: Relevant Log Output + description: Please copy and paste any relevant log output + render: shell + + - type: textarea + id: additional + attributes: + label: Additional Context + description: Add any other context about the problem here diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml new file mode 100644 index 0000000..c9376f7 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -0,0 +1,8 @@ +blank_issues_enabled: false +contact_links: + - name: 💬 Questions & Discussions + url: https://github.com/zipstack/helm-values-manager/discussions + about: Please ask and answer questions here. + - name: 📖 Documentation + url: https://github.com/zipstack/helm-values-manager/tree/main/docs + about: Check our documentation before creating an issue. diff --git a/.github/ISSUE_TEMPLATE/documentation.yml b/.github/ISSUE_TEMPLATE/documentation.yml new file mode 100644 index 0000000..9809ad8 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/documentation.yml @@ -0,0 +1,64 @@ +name: 📚 Documentation +description: Suggest improvements or report issues in documentation +title: "[Docs]: " +labels: ["documentation"] +body: + - type: markdown + attributes: + value: | + Thanks for helping us improve our documentation! + + - type: dropdown + id: doc-type + attributes: + label: Documentation Type + description: What type of documentation needs attention? + options: + - README + - API Documentation + - Installation Guide + - Usage Examples + - Architecture/Design Docs + - Contributing Guide + - Other + validations: + required: true + + - type: textarea + id: current-issue + attributes: + label: Current Documentation Issue + description: What's missing, unclear, or incorrect in the current documentation? + placeholder: The current documentation lacks information about... + validations: + required: true + + - type: textarea + id: suggested-changes + attributes: + label: Suggested Changes + description: Describe your suggested changes or additions + placeholder: | + The documentation should include: + 1. ... + 2. ... + validations: + required: true + + - type: checkboxes + id: checklist + attributes: + label: Checklist + options: + - label: I've checked that this isn't already documented somewhere else + required: true + - label: I've provided specific suggestions for improvement + required: true + - label: I've included examples where relevant + required: false + + - type: textarea + id: additional + attributes: + label: Additional Context + description: Add any other context about the documentation request here diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml new file mode 100644 index 0000000..7ce6b12 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -0,0 +1,70 @@ +name: ✨ Feature Request +description: Suggest an idea for this project +title: "[Feature]: " +labels: ["enhancement"] +body: + - type: markdown + attributes: + value: | + Thanks for taking the time to suggest a new feature! + + - type: textarea + id: problem + attributes: + label: Problem Statement + description: Is your feature request related to a problem? Please describe. + placeholder: I'm always frustrated when [...] + validations: + required: true + + - type: textarea + id: solution + attributes: + label: Proposed Solution + description: Describe the solution you'd like + placeholder: | + A clear and concise description of what you want to happen. + Include any specific implementation details you have in mind. + validations: + required: true + + - type: textarea + id: alternatives + attributes: + label: Alternative Solutions + description: Describe alternatives you've considered + placeholder: What other approaches could solve this problem? + validations: + required: false + + - type: checkboxes + id: requirements + attributes: + label: Requirements + description: Please confirm these requirements + options: + - label: This feature aligns with the project's scope and goals + required: true + - label: I've checked that this feature doesn't already exist + required: true + - label: I've searched for existing feature requests + required: true + + - type: textarea + id: acceptance-criteria + attributes: + label: Acceptance Criteria + description: List the requirements that should be met + placeholder: | + - [ ] Feature does X when Y + - [ ] Handles error case Z + - [ ] Documentation updated + - [ ] Tests added + validations: + required: true + + - type: textarea + id: additional + attributes: + label: Additional Context + description: Add any other context about the feature request here diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..1f56f78 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,47 @@ +## Related Issues + +Fixes # + +## Changes Made + +- +- +- + +## Testing Done + +- [ ] Added unit tests +- [ ] Added integration tests +- [ ] Manually tested +- [ ] Updated test documentation + +## Checklist + +- [ ] I have read the CONTRIBUTING guidelines +- [ ] My code follows the project's style guidelines +- [ ] I have added tests that prove my fix/feature works +- [ ] I have updated the documentation accordingly +- [ ] All new and existing tests passed +- [ ] My commits follow the project's commit message convention + +## Type of Change + +- [ ] Feature +- [ ] Bug Fix +- [ ] Documentation +- [ ] Performance Improvement +- [ ] Code Style Update +- [ ] Refactoring +- [ ] CI/CD +- [ ] Other + +## Breaking Change + +- [ ] No +- [ ] Yes (please describe below) + +### Breaking Change Description + + +## Additional Information + diff --git a/.github/pull_request_template.yml b/.github/pull_request_template.yml new file mode 100644 index 0000000..e4b6dc5 --- /dev/null +++ b/.github/pull_request_template.yml @@ -0,0 +1,101 @@ +name: Pull Request +description: Create a pull request +title: "[PR]: " +body: + - type: markdown + attributes: + value: | + Thanks for creating this pull request! Please ensure you've read our contributing guidelines. + + - type: input + id: related-issues + attributes: + label: Related Issues + description: List any related issues this PR addresses + placeholder: "#123, #456" + validations: + required: true + + - type: textarea + id: changes + attributes: + label: Changes Made + description: Describe the changes you've made + placeholder: | + - Added feature X + - Fixed bug Y + - Updated documentation for Z + validations: + required: true + + - type: textarea + id: testing + attributes: + label: Testing Done + description: Describe the testing you've performed + placeholder: | + - Added unit tests for X + - Added integration tests for Y + - Manually tested scenario Z + validations: + required: true + + - type: checkboxes + id: checklist + attributes: + label: Pull Request Checklist + description: Please check all that apply + options: + - label: I have read the CONTRIBUTING guidelines + required: true + - label: My code follows the project's style guidelines + required: true + - label: I have added tests that prove my fix/feature works + required: true + - label: I have updated the documentation accordingly + required: true + - label: All new and existing tests passed + required: true + - label: My commits follow the project's commit message convention + required: true + + - type: dropdown + id: type + attributes: + label: Type of Change + description: What type of change does this PR introduce? + options: + - Feature + - Bug Fix + - Documentation + - Performance Improvement + - Code Style Update + - Refactoring + - CI/CD + - Other + validations: + required: true + + - type: dropdown + id: breaking + attributes: + label: Breaking Change + description: Does this PR introduce a breaking change? + options: + - "No" + - "Yes" + validations: + required: true + + - type: textarea + id: breaking-description + attributes: + label: Breaking Change Description + description: If this PR introduces a breaking change, describe it and migration steps + placeholder: Describe the breaking changes and how to migrate... + + - type: textarea + id: additional + attributes: + label: Additional Information + description: Add any other context about the PR here diff --git a/.gitignore b/.gitignore index 2d96195..c893026 100644 --- a/.gitignore +++ b/.gitignore @@ -184,3 +184,6 @@ bin/ .DS_Store .AppleDouble .LSOverride + +# windsurf rules +.windsurfrules diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 541e283..dd2cae5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -10,27 +10,61 @@ We love your input! We want to make contributing to Helm Values Manager as easy ## Development Process -We use GitHub to host code, to track issues and feature requests, as well as accept pull requests. - -1. Fork the repo and create your branch from `main`. -2. If you've added code that should be tested, add tests. -3. If you've changed APIs, update the documentation. -4. Ensure the test suite passes. -5. Make sure your code passes all code quality checks. -6. Issue that pull request! +Before starting work: +1. Check if a GitHub issue exists for your feature/bug + - If not, create a new issue using the appropriate template: + - Bug Report: For reporting bugs + - Feature Request: For proposing new features + - Documentation: For documentation improvements + - If yes, comment on the issue to let others know you're working on it + +For contributors with write access: +1. Create a new branch from `main` using the issue number (see Branch Naming below) +2. Make your changes following our development guidelines +3. Create a pull request + +For contributors without write access: +1. Fork the repository +2. Create a new branch in your fork + - We recommend creating a branch even in your fork to: + - Keep your fork's main branch clean and in sync with upstream + - Work on multiple issues simultaneously + - Make PR feedback changes easier +3. Make your changes in the branch +4. Create a pull request from your fork's branch to our main repository + +For all contributors: +1. If you've added code that should be tested, add tests +2. If you've changed APIs, update the documentation +3. Ensure the test suite passes +4. Make sure your code passes all code quality checks ## Version Control Guidelines -### Branch Naming -- Feature branches: `feature/short-description` -- Bug fixes: `fix/issue-description` -- Documentation: `docs/what-changed` -- Release branches: `release/version-number` +### Branch Naming Convention + +All branches should follow this naming convention: + +``` +- +``` + +For example: +- `123-add-aws-secrets-backend` +- `456-fix-path-validation` +- `789-update-documentation` + +Note: This naming convention is required for branches in the main repository. When working in your own fork, we recommend: +1. Creating a dedicated branch for each issue (don't work in main) +2. Following the same naming convention for consistency +3. Keeping your fork's main branch clean for easy upstream syncing ### Commit Messages -Follow the [Conventional Commits](https://www.conventionalcommits.org/) specification: + +Follow the [Conventional Commits](https://www.conventionalcommits.org/) specification with issue number references: + ``` -[optional scope]: +[optional scope]: # [optional body] @@ -38,25 +72,33 @@ Follow the [Conventional Commits](https://www.conventionalcommits.org/) specific ``` Types: -- `feat`: New feature -- `fix`: Bug fix +- `feat`: A new feature +- `fix`: A bug fix - `docs`: Documentation only changes - `style`: Changes that do not affect the meaning of the code -- `refactor`: Code change that neither fixes a bug nor adds a feature +- `refactor`: A code change that neither fixes a bug nor adds a feature - `test`: Adding missing tests or correcting existing tests - `chore`: Changes to the build process or auxiliary tools -Example: +Examples: +``` +feat: #123 add AWS Secrets backend +fix: #456 handle empty paths correctly +docs: #789 update installation guide +test: #234 add integration tests for Value class ``` -feat(value): add support for remote storage backends - -- Implement ValueBackend interface -- Add AWS Secrets Manager backend -- Add Azure Key Vault backend -Closes #123 +For multiple issues, include all issue numbers: +``` +fix: #111 #222 resolve path handling edge cases ``` +Note: +- Always include the issue number with a `#` prefix +- Place the issue number before the description +- Multiple issues should be space-separated +- The description should still be clear without the issue number + ## Important Documentation Before contributing, please review these important documents: diff --git a/README.md b/README.md index 96dd92d..bb20cfd 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=Zipstack_helm-values-manager&metric=alert_status)](https://sonarcloud.io/summary/new_code?id=Zipstack_helm-values-manager) [![Build Status](https://github.com/Zipstack/helm-values-manager/actions/workflows/test.yml/badge.svg)](https://github.com/Zipstack/helm-values-manager/actions/workflows/test.yml) +[![pre-commit.ci status](https://results.pre-commit.ci/badge/github/Zipstack/helm-values-manager/main.svg)](https://results.pre-commit.ci/latest/github/Zipstack/helm-values-manager/main) [![Security](https://github.com/Zipstack/helm-values-manager/actions/workflows/github-code-scanning/codeql/badge.svg)](https://github.com/Zipstack/helm-values-manager/actions/workflows/github-code-scanning/codeql) [![Coverage](https://sonarcloud.io/api/project_badges/measure?project=Zipstack_helm-values-manager&metric=coverage)](https://sonarcloud.io/summary/new_code?id=Zipstack_helm-values-manager) [![Duplicated Lines (%)](https://sonarcloud.io/api/project_badges/measure?project=Zipstack_helm-values-manager&metric=duplicated_lines_density)](https://sonarcloud.io/summary/new_code?id=Zipstack_helm-values-manager) diff --git a/docs/ADRs/006-helm-style-logging.md b/docs/ADRs/006-helm-style-logging.md new file mode 100644 index 0000000..fb4a920 --- /dev/null +++ b/docs/ADRs/006-helm-style-logging.md @@ -0,0 +1,125 @@ +# ADR-006: Helm-Style Logging System + +## Status +Accepted + +## Context +As a Helm plugin, our application needs to follow Helm's logging conventions for consistency and user experience. Key considerations include: + +1. Debug output control via environment variables +2. Error message formatting +3. Performance in logging operations +4. Integration with Helm's output conventions +5. Testing and verification of log output + +## Decision + +### 1. Implement HelmLogger Class +Create a dedicated logger class that follows Helm plugin conventions: + +```python +class HelmLogger: + @staticmethod + def debug(msg: str, *args: Any) -> None: + if os.environ.get('HELM_DEBUG'): + if args: + msg = msg % args + print("[debug] %s" % msg, file=sys.stderr) + + @staticmethod + def error(msg: str, *args: Any) -> None: + if args: + msg = msg % args + print("Error: %s" % msg, file=sys.stderr) +``` + +### 2. Key Design Decisions + +1. **Helm Convention Compliance** + - Use stderr for all output + - Prefix debug messages with "[debug]" + - Prefix error messages with "Error:" + - Control debug output via HELM_DEBUG environment variable + +2. **Performance Optimization** + - Use string formatting instead of f-strings for consistent behavior + - Lazy evaluation of debug messages + - No string concatenation in critical paths + +3. **Global Instance Pattern** + - Provide a global logger instance + - Enable consistent logging across the codebase + - Avoid passing logger instances + +4. **Testing Support** + - Mockable stderr for testing + - Environment variable control in tests + - String format verification + +### 3. Usage Pattern +```python +from helm_values_manager.utils.logger import logger + +def some_function(): + logger.debug("Processing %s", value) + try: + # do something + logger.debug("Success!") + except Exception as e: + logger.error("Failed: %s", str(e)) +``` + +## Consequences + +### Positive +1. **Consistent User Experience** + - Matches Helm's output format + - Familiar debug control mechanism + - Clear error messages + +2. **Performance** + - Efficient string formatting + - Debug messages skipped when disabled + - Minimal memory overhead + +3. **Maintainability** + - Centralized logging logic + - Easy to modify output format + - Simple testing approach + +4. **Debugging** + - Clear debug output control + - Consistent message format + - Environment-based control + +### Negative +1. **Limited Flexibility** + - Fixed output format + - No log levels beyond debug/error + - No log file support + +2. **Global State** + - Global logger instance + - Potential for test interference + - Need careful test isolation + +## Implementation Notes + +1. **Testing** +```python +def test_debug_output(): + stderr = StringIO() + with mock.patch.dict(os.environ, {'HELM_DEBUG': '1'}), \ + mock.patch('helm_values_manager.utils.logger.sys.stderr', stderr): + logger.debug("Test message") + assert stderr.getvalue() == "[debug] Test message\n" +``` + +2. **Integration** +```python +# In PathData class +def validate(self): + logger.debug("Validating PathData for path: %s", self.path) + if not self.is_valid(): + logger.error("Invalid PathData: %s", self.path) +``` diff --git a/docs/ADRs/README.md b/docs/ADRs/README.md new file mode 100644 index 0000000..e9bb760 --- /dev/null +++ b/docs/ADRs/README.md @@ -0,0 +1,67 @@ +# Architecture Decision Records (ADRs) + +This directory contains Architecture Decision Records (ADRs) for the Helm Values Manager project. + +## What is an ADR? +An Architecture Decision Record (ADR) is a document that captures an important architectural decision made along with its context and consequences. + +## ADR Index + +### [ADR-001: Helm Values Manager as a Helm Plugin](001-helm-values-manager.md) +- **Status**: Accepted +- **Context**: Need for managing configurations and secrets across multiple Kubernetes deployments +- **Decision**: Implement as a Helm plugin in Python with key-value storage model +- **Impact**: Defines core architecture and integration approach + +### [ADR-002: Config Path and Storage Design](002-config-path-and-storage-design.md) +- **Status**: Accepted +- **Context**: Need for separate config definition from value storage +- **Decision**: Implement path map and standardized command pattern +- **Impact**: Establishes core data structures and file operations +- **Dependencies**: ADR-001 + +### [ADR-003: Unified Path and Value Storage](003-unified-path-value-storage.md) +- **Status**: Accepted +- **Context**: Complexity from separate path and value storage +- **Decision**: Unified dictionary structure for paths and values +- **Impact**: Simplifies storage and improves consistency +- **Dependencies**: ADR-002 + +### [ADR-004: Value Resolution Strategy](004-value-resolution-strategy.md) +- **Status**: Accepted +- **Context**: Need for clear value resolution in unified storage +- **Decision**: Introduce Value class for encapsulation +- **Impact**: Clarifies value handling and storage strategy +- **Dependencies**: ADR-003 + +### [ADR-005: Unified Backend Approach](005-unified-backend-approach.md) +- **Status**: Proposed +- **Context**: Split logic in Value class for different storage types +- **Decision**: Remove storage type distinction, use SimpleValueBackend +- **Impact**: Simplifies Value class and unifies storage interface +- **Dependencies**: ADR-004 + +### [ADR-006: Helm-Style Logging System](006-helm-style-logging.md) +- **Status**: Accepted +- **Context**: Need for consistent logging following Helm conventions +- **Decision**: Implement HelmLogger with Helm-style output +- **Impact**: Ensures consistent user experience and debugging +- **Dependencies**: ADR-001 + +## ADR Template +For new ADRs, use this template: +```markdown +# ADR-NNN: Title + +## Status +[Proposed | Accepted | Deprecated | Superseded] + +## Context +What is the issue that we're seeing that is motivating this decision or change? + +## Decision +What is the change that we're proposing and/or doing? + +## Consequences +What becomes easier or more difficult to do because of this change? +``` diff --git a/docs/Design/low-level-design.md b/docs/Design/low-level-design.md index 9cf09bc..a85804f 100644 --- a/docs/Design/low-level-design.md +++ b/docs/Design/low-level-design.md @@ -21,8 +21,14 @@ classDiagram } class PathData { + +str path +Dict metadata +Dict~str,Value~ values + +validate() None + +add_value(environment: str, value: Value) None + +get_value(environment: str) Optional[Value] + +to_dict() Dict + +from_dict(data: Dict, create_value_fn) PathData } class Value { @@ -85,6 +91,11 @@ classDiagram +validate_auth_config(auth_config: dict) None } + class HelmLogger { + +debug(msg: str, *args: Any) None + +error(msg: str, *args: Any) None + } + HelmValuesConfig "1" *-- "*" ConfigValue HelmValuesConfig "1" *-- "*" Deployment HelmValuesConfig "1" *-- "*" PathData @@ -99,32 +110,32 @@ classDiagram ### 2. Value Management -The system uses a unified approach to value storage and resolution through the `Value` class: +The system manages configuration values through a hierarchy of classes: -```python -class Value: - def __init__(self, path: str, environment: str, - backend: ValueBackend): - self.path = path - self.environment = environment - self._backend = backend - - def get(self) -> str: - """Get the resolved value""" - return self._backend.get_value(self.path, self.environment) - - def set(self, value: str) -> None: - """Set the value""" - if not isinstance(value, str): - raise ValueError("Value must be a string") - self._backend.set_value(self.path, self.environment, value) -``` +1. **HelmValuesConfig** + - Top-level configuration manager + - Maintains mapping of paths to PathData instances + - Handles backend selection based on value sensitivity + - Manages deployments and their configurations + +2. **PathData** + - Represents a single configuration path and its properties + - Owns the configuration path and ensures consistency + - Stores metadata (description, required status, sensitivity) + - Manages environment-specific values through Value instances + - Validates path consistency between itself and its Values + - Delegates actual value storage to Value instances -Key features: -- Encapsulated value resolution logic -- Unified interface for all storage backends +3. **Value** + - Handles actual value storage and retrieval + - Uses appropriate backend for storage operations + - Maintains reference to its path and environment + +This hierarchy ensures: - Clear separation of concerns -- Type-safe value handling +- Consistent path handling across the system +- Proper validation at each level +- Flexible backend selection based on value sensitivity ### 3. Command Pattern @@ -249,6 +260,31 @@ The configuration follows the v1 schema: - Backup before writes - Atomic updates +## Logging System + +The logging system follows Helm plugin conventions and provides consistent output formatting: + +1. **HelmLogger Class** + - Provides debug and error logging methods + - Follows Helm output conventions + - Uses stderr for all output + - Controls debug output via HELM_DEBUG environment variable + +2. **Global Logger Instance** + - Available via `from helm_values_manager.utils.logger import logger` + - Ensures consistent logging across all components + - Simplifies testing with mock support + +3. **Performance Features** + - Uses string formatting for efficiency + - Lazy evaluation of debug messages + - Minimal memory overhead + +4. **Testing Support** + - Mockable stderr output + - Environment variable control + - String format verification + ## Benefits of This Design 1. **Separation of Concerns** diff --git a/docs/Development/tasks.md b/docs/Development/tasks.md index 809997f..642f87e 100644 --- a/docs/Development/tasks.md +++ b/docs/Development/tasks.md @@ -22,14 +22,14 @@ - [x] Add support for sensitive field handling ### PathData Class Implementation -- [ ] Create PathData class - - [ ] Add metadata dictionary support - - [ ] Add values dictionary for Value objects - - [ ] Implement validation methods -- [ ] Add serialization support - - [ ] Implement to_dict() method - - [ ] Implement from_dict() method - - [ ] Add tests for serialization +- [x] Create PathData class + - [x] Add metadata dictionary support + - [x] Add values dictionary for Value objects + - [x] Implement validation methods +- [x] Add serialization support + - [x] Implement to_dict() method + - [x] Implement from_dict() method + - [x] Add tests for serialization ### HelmValuesConfig Refactoring - [ ] Remove PlainTextBackend references @@ -75,13 +75,13 @@ - [ ] Add command-specific validation ### Testing Infrastructure -- [ ] Set up test infrastructure - - [ ] Add pytest configuration - - [ ] Set up test fixtures - - [ ] Add mock backends for testing -- [ ] Add unit tests - - [ ] Value class tests - - [ ] PathData class tests +- [x] Set up test infrastructure + - [x] Add pytest configuration + - [x] Set up test fixtures + - [x] Add mock backends for testing +- [x] Add unit tests + - [x] Value class tests + - [x] PathData class tests - [ ] HelmValuesConfig tests - [ ] Backend tests - [ ] Command tests @@ -90,6 +90,22 @@ - [ ] Backend integration tests - [ ] File operation tests +### Logging System +- [x] Implement Helm-style logger + - [x] Create HelmLogger class + - [x] Add debug and error methods + - [x] Follow Helm output conventions + - [x] Add HELM_DEBUG support +- [x] Add comprehensive tests + - [x] Test debug output control + - [x] Test error formatting + - [x] Test string formatting + - [x] Test environment handling +- [x] Update components to use logger + - [x] Update PathData class + - [x] Update Value class + - [x] Add logging documentation + ### Documentation - [ ] Update API documentation - [ ] Document Value class diff --git a/helm_values_manager/models/path_data.py b/helm_values_manager/models/path_data.py new file mode 100644 index 0000000..cce318c --- /dev/null +++ b/helm_values_manager/models/path_data.py @@ -0,0 +1,187 @@ +""" +PathData class for managing path-specific metadata and values. + +This module contains the PathData class which is responsible for managing +metadata and values associated with a specific configuration path. +""" + +from typing import Any, Dict, Iterator, Optional + +from ..utils.logger import logger +from .value import Value + + +class PathData: + """ + Class representing metadata and values for a specific configuration path. + + This class manages both the metadata (like description, required status, + sensitivity) and the actual values (as Value objects) for a configuration path. + + Attributes: + path (str): The configuration path (e.g., "app.replicas") + metadata (Dict[str, Any]): Dictionary containing path metadata like + description, required status, and sensitivity. + """ + + def __init__(self, path: str, metadata: Dict[str, Any]) -> None: + """ + Initialize a new PathData instance. + + Args: + path (str): The configuration path + metadata (Dict[str, Any]): Dictionary containing path metadata. + Expected keys: description (str), required (bool), sensitive (bool) + """ + logger.debug("Creating new PathData instance for path: %s", path) + self.path = path + self.metadata = metadata + self._values: Dict[str, Value] = {} + + def validate(self) -> None: + """ + Validate the PathData instance. + + Ensures that: + 1. Required metadata fields are present + 2. Metadata fields have correct types + 3. If path is marked as required, all environments have values + 4. All Value instances use the same path as this PathData + + Raises: + ValueError: If validation fails + """ + logger.debug("Validating PathData for path: %s", self.path) + + # Validate metadata structure + required_fields = {"description", "required", "sensitive"} + missing_fields = required_fields - set(self.metadata.keys()) + if missing_fields: + logger.error("Missing required metadata fields for path %s: %s", self.path, missing_fields) + raise ValueError(f"Missing required metadata fields: {missing_fields}") + + # Validate metadata types + if not isinstance(self.metadata["description"], (str, type(None))): + logger.error("Invalid description type for path %s", self.path) + raise ValueError("Description must be a string or None") + if not isinstance(self.metadata["required"], bool): + logger.error("Invalid required flag type for path %s", self.path) + raise ValueError("Required flag must be a boolean") + if not isinstance(self.metadata["sensitive"], bool): + logger.error("Invalid sensitive flag type for path %s", self.path) + raise ValueError("Sensitive flag must be a boolean") + + # Validate path consistency + for env, value in self._values.items(): + if value.path != self.path: + logger.error("Path mismatch for environment %s: %s != %s", env, value.path, self.path) + raise ValueError(f"Value for environment {env} has inconsistent path: {value.path} != {self.path}") + + # If path is required, ensure all environments have values + if self.metadata["required"]: + logger.debug("Path %s is marked as required", self.path) + # Note: The actual environment validation would need to be done + # at a higher level since PathData doesn't know about all environments + + def set_value(self, environment: str, value: Value) -> None: + """ + Set a Value object for a specific environment. + + If a value already exists for the environment, it will be replaced. + + Args: + environment (str): The environment name + value (Value): The Value object to associate with the environment + + Raises: + ValueError: If the Value object's path doesn't match this PathData's path + """ + logger.debug("Setting value for path %s in environment %s", self.path, environment) + + if value.path != self.path: + logger.error("Value path %s doesn't match PathData path %s", value.path, self.path) + raise ValueError(f"Value path {value.path} doesn't match PathData path {self.path}") + + was_update = environment in self._values + self._values[environment] = value + logger.debug( + "%s value for path %s in environment %s", "Updated" if was_update else "Added", self.path, environment + ) + + def get_value(self, environment: str) -> Optional[Value]: + """ + Get the Value object for a specific environment. + + Args: + environment (str): The environment to get the value for + + Returns: + Optional[Value]: The Value object if it exists, None otherwise + """ + logger.debug("Getting value for path %s in environment %s", self.path, environment) + value = self._values.get(environment) + if value is None: + logger.debug("No value found for path %s in environment %s", self.path, environment) + return value + + def get_environments(self) -> Iterator[str]: + """ + Get an iterator over all environments that have values. + + Returns: + Iterator[str]: Iterator of environment names + """ + logger.debug("Getting environments for path %s", self.path) + return iter(self._values.keys()) + + def to_dict(self) -> Dict[str, Any]: + """ + Convert the PathData instance to a dictionary. + + Returns: + Dict[str, Any]: Dictionary representation of the PathData instance + """ + logger.debug("Converting PathData to dict for path %s", self.path) + return { + "path": self.path, + "metadata": self.metadata, + "values": {env: value.to_dict() for env, value in self._values.items()}, + } + + @classmethod + def from_dict(cls, data: Dict[str, Any], create_value_fn) -> "PathData": + """ + Create a PathData instance from a dictionary. + + Args: + data (Dict[str, Any]): Dictionary containing PathData data + create_value_fn: Function to create Value objects from dict data. + Should accept (path: str, environment: str, data: Dict) as arguments. + + Returns: + PathData: New PathData instance + + Raises: + ValueError: If the dictionary structure is invalid + """ + if not isinstance(data, dict): + logger.error("Invalid data type provided: %s", type(data)) + raise ValueError("Data must be a dictionary") + + logger.debug("Creating PathData from dict with path: %s", data.get("path")) + + required_keys = {"path", "metadata", "values"} + if not all(key in data for key in required_keys): + missing = required_keys - set(data.keys()) + logger.error("Missing required keys in data: %s", missing) + raise ValueError(f"Missing required keys: {missing}") + + instance = cls(data["path"], data["metadata"]) + + # Reconstruct values + for env, value_data in data["values"].items(): + logger.debug("Creating value for environment %s", env) + value = create_value_fn(data["path"], env, value_data) + instance.set_value(env, value) + + return instance diff --git a/helm_values_manager/models/value.py b/helm_values_manager/models/value.py index f66820e..0aa5206 100644 --- a/helm_values_manager/models/value.py +++ b/helm_values_manager/models/value.py @@ -9,6 +9,7 @@ from typing import Any, Dict from ..backends.base import ValueBackend +from ..utils.logger import logger @dataclass @@ -26,6 +27,10 @@ class Value: environment: str _backend: ValueBackend + def __post_init__(self): + """Post-initialization validation and logging.""" + logger.debug("Created Value instance for path %s in environment %s", self.path, self.environment) + def get(self) -> str: """ Get the resolved value. @@ -37,7 +42,14 @@ def get(self) -> str: ValueError: If value doesn't exist RuntimeError: If backend operation fails """ - return self._backend.get_value(self.path, self.environment) + logger.debug("Getting value for path %s in environment %s", self.path, self.environment) + try: + value = self._backend.get_value(self.path, self.environment) + logger.debug("Successfully retrieved value for path %s", self.path) + return value + except Exception as e: + logger.error("Failed to get value for path %s in environment %s: %s", self.path, self.environment, str(e)) + raise def set(self, value: str) -> None: """ @@ -50,23 +62,28 @@ def set(self, value: str) -> None: ValueError: If value is not a string RuntimeError: If backend operation fails """ + logger.debug("Setting value for path %s in environment %s", self.path, self.environment) + if not isinstance(value, str): + logger.error("Invalid value type for path %s: expected str, got %s", self.path, type(value)) raise ValueError("Value must be a string") - self._backend.set_value(self.path, self.environment, value) + + try: + self._backend.set_value(self.path, self.environment, value) + logger.debug("Successfully set value for path %s", self.path) + except Exception as e: + logger.error("Failed to set value for path %s in environment %s: %s", self.path, self.environment, str(e)) + raise def to_dict(self) -> Dict[str, Any]: """ - Serialize the value configuration to a dictionary. + Convert the Value instance to a dictionary. Returns: - dict: Serialized representation of the value containing: - - path: The configuration path - - environment: The environment name + Dict[str, Any]: Dictionary representation of the Value instance """ - return { - "path": self.path, - "environment": self.environment, - } + logger.debug("Converting Value to dict for path %s", self.path) + return {"path": self.path, "environment": self.environment, "backend_type": self._backend.backend_type} @staticmethod def from_dict(data: Dict[str, Any], backend: ValueBackend) -> "Value": diff --git a/helm_values_manager/utils/__init__.py b/helm_values_manager/utils/__init__.py new file mode 100644 index 0000000..62ae199 --- /dev/null +++ b/helm_values_manager/utils/__init__.py @@ -0,0 +1 @@ +"""Utility modules for Helm Values Manager.""" diff --git a/helm_values_manager/utils/logger.py b/helm_values_manager/utils/logger.py new file mode 100644 index 0000000..9d80bdb --- /dev/null +++ b/helm_values_manager/utils/logger.py @@ -0,0 +1,53 @@ +""" +Logger utility for Helm Values Manager. + +This module provides a Helm-style logger that respects HELM_DEBUG environment variable +and follows Helm plugin conventions for output. +""" + +import os +import sys +from typing import Any + + +class HelmLogger: + """ + Logger class that follows Helm plugin conventions. + + This logger: + 1. Writes debug messages only when HELM_DEBUG is set + 2. Writes all messages to stderr (Helm convention) + 3. Uses string formatting for better performance + 4. Provides consistent error and debug message formatting + """ + + @staticmethod + def debug(msg: str, *args: Any) -> None: + """ + Print debug message if HELM_DEBUG is set. + + 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) + + @staticmethod + def error(msg: str, *args: Any) -> None: + """ + Print error message to stderr. + + Args: + msg: Message with optional string format placeholders + args: Values to substitute in the message + """ + if args: + msg = msg % args + print("Error: %s" % msg, file=sys.stderr) + + +# Global logger instance +logger = HelmLogger() diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 0000000..f0eb052 --- /dev/null +++ b/tests/__init__.py @@ -0,0 +1 @@ +"""Test package for Helm Values Manager.""" diff --git a/tests/unit/models/test_path_data.py b/tests/unit/models/test_path_data.py new file mode 100644 index 0000000..b3f2d13 --- /dev/null +++ b/tests/unit/models/test_path_data.py @@ -0,0 +1,161 @@ +"""Tests for the PathData class.""" + +from unittest.mock import Mock + +import pytest + +from helm_values_manager.models.path_data import PathData +from helm_values_manager.models.value import Value + + +@pytest.fixture +def mock_value(): + """Create a mock Value instance.""" + mock_backend = Mock() + mock_backend.get_value.return_value = "test_value" + return Value(path="test.path", environment="test", _backend=mock_backend) + + +@pytest.fixture +def valid_metadata(): + """Create valid metadata for testing.""" + return {"description": "Test description", "required": True, "sensitive": False} + + +@pytest.fixture +def path_data(valid_metadata): + """Create a PathData instance with valid metadata.""" + return PathData("test.path", valid_metadata) + + +def test_init_with_valid_metadata(valid_metadata): + """Test PathData initialization with valid metadata.""" + path_data = PathData("test.path", valid_metadata) + assert path_data.path == "test.path" + assert path_data.metadata == valid_metadata + assert list(path_data.get_environments()) == [] + + +def test_init_with_none_description(): + """Test PathData initialization with None description.""" + metadata = {"description": None, "required": True, "sensitive": False} + path_data = PathData("test.path", metadata) + assert path_data.metadata["description"] is None + + +def test_validate_missing_metadata(): + """Test validate() with missing metadata fields.""" + metadata = {"description": "Test"} # Missing required and sensitive + path_data = PathData("test.path", metadata) + with pytest.raises(ValueError, match="Missing required metadata fields"): + path_data.validate() + + +def test_validate_invalid_metadata_types(): + """Test validate() with invalid metadata types.""" + test_cases = [ + { + "metadata": {"description": 123, "required": True, "sensitive": False}, + "match": "Description must be a string or None", + }, + { + "metadata": {"description": "Test", "required": "True", "sensitive": False}, + "match": "Required flag must be a boolean", + }, + { + "metadata": {"description": "Test", "required": True, "sensitive": "False"}, + "match": "Sensitive flag must be a boolean", + }, + ] + + for case in test_cases: + path_data = PathData("test.path", case["metadata"]) + with pytest.raises(ValueError, match=case["match"]): + path_data.validate() + + +def test_validate_path_consistency(path_data): + """Test validate() with inconsistent paths.""" + mock_backend = Mock() + inconsistent_value = Value(path="different.path", environment="test", _backend=mock_backend) + # Access private attribute for testing + path_data._values["test"] = inconsistent_value + with pytest.raises(ValueError, match="Value for environment test has inconsistent path"): + path_data.validate() + + +def test_set_value(path_data, mock_value): + """Test setting a Value object.""" + # Test adding a new value + path_data.set_value("test", mock_value) + assert path_data.get_value("test") == mock_value + assert list(path_data.get_environments()) == ["test"] + + # Test updating an existing value + new_mock_value = Value(path="test.path", environment="test", _backend=Mock()) + path_data.set_value("test", new_mock_value) + assert path_data.get_value("test") == new_mock_value + + +def test_set_value_with_wrong_path(path_data): + """Test setting a Value object with wrong path.""" + mock_backend = Mock() + wrong_path_value = Value(path="wrong.path", environment="test", _backend=mock_backend) + with pytest.raises(ValueError, match="Value path wrong.path doesn't match PathData path test.path"): + path_data.set_value("test", wrong_path_value) + + +def test_get_value(path_data, mock_value): + """Test getting a Value object.""" + path_data.set_value("test", mock_value) + assert path_data.get_value("test") == mock_value + assert path_data.get_value("nonexistent") is None + + +def test_get_environments(path_data, mock_value): + """Test getting environment names.""" + path_data.set_value("test1", mock_value) + path_data.set_value("test2", mock_value) + assert set(path_data.get_environments()) == {"test1", "test2"} + + +def test_to_dict(path_data, mock_value): + """Test converting PathData to dictionary.""" + path_data.set_value("test", mock_value) + result = path_data.to_dict() + + assert result["path"] == "test.path" + assert result["metadata"] == path_data.metadata + assert "test" in result["values"] + assert result["values"]["test"] == mock_value.to_dict() + + +def test_from_dict_valid_data(valid_metadata, mock_value): + """Test creating PathData from valid dictionary data.""" + data = { + "path": "test.path", + "metadata": valid_metadata, + "values": {"test": {"path": "test.path", "environment": "test", "backend_type": "mock"}}, + } + + def create_value_fn(path, env, data): + return mock_value + + path_data = PathData.from_dict(data, create_value_fn) + assert path_data.path == "test.path" + assert path_data.metadata == valid_metadata + assert path_data.get_value("test") == mock_value + + +def test_from_dict_invalid_data(): + """Test from_dict() with invalid data.""" + test_cases = [ + ("not_a_dict", "Data must be a dictionary"), + ({"metadata": {}, "values": {}}, "Missing required keys"), # Missing path + ({"path": "test.path", "values": {}}, "Missing required keys"), # Missing metadata + ({"path": "test.path", "metadata": {}}, "Missing required keys"), # Missing values + ] + + for data, error_match in test_cases: + with pytest.raises(ValueError, match=error_match): + PathData.from_dict(data, lambda p, e, d: None) diff --git a/tests/unit/models/test_value.py b/tests/unit/models/test_value.py index 271c61a..701010b 100644 --- a/tests/unit/models/test_value.py +++ b/tests/unit/models/test_value.py @@ -47,9 +47,10 @@ def test_set_invalid_type(mock_backend): def test_to_dict(mock_backend): """Test serializing a value.""" + mock_backend.backend_type = "mock" value = Value(path="app.replicas", environment="dev", _backend=mock_backend) data = value.to_dict() - assert data == {"path": "app.replicas", "environment": "dev"} + assert data == {"path": "app.replicas", "environment": "dev", "backend_type": "mock"} def test_from_dict(mock_backend): diff --git a/tests/unit/utils/__init__.py b/tests/unit/utils/__init__.py new file mode 100644 index 0000000..45341ee --- /dev/null +++ b/tests/unit/utils/__init__.py @@ -0,0 +1 @@ +"""Unit tests for utility modules.""" diff --git a/tests/unit/utils/test_logger.py b/tests/unit/utils/test_logger.py new file mode 100644 index 0000000..aa4c38b --- /dev/null +++ b/tests/unit/utils/test_logger.py @@ -0,0 +1,75 @@ +"""Tests for the HelmLogger utility.""" + +import os +from io import StringIO +from unittest import mock + +import pytest + +from helm_values_manager.utils.logger import HelmLogger + + +@pytest.fixture +def logger(): + """Create a HelmLogger instance.""" + return HelmLogger() + + +def test_debug_with_helm_debug_enabled(logger): + """Test debug output when HELM_DEBUG is set.""" + stderr = StringIO() + with ( + mock.patch.dict(os.environ, {"HELM_DEBUG": "1"}), + mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr), + ): + logger.debug("Test message") + assert stderr.getvalue() == "[debug] Test message\n" + + +def test_debug_with_helm_debug_disabled(logger): + """Test debug output when HELM_DEBUG is not set.""" + stderr = StringIO() + with mock.patch.dict(os.environ, {}), mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr): + logger.debug("Test message") + assert stderr.getvalue() == "" + + +def test_debug_with_formatting(logger): + """Test debug output with string formatting.""" + stderr = StringIO() + with ( + mock.patch.dict(os.environ, {"HELM_DEBUG": "1"}), + mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr), + ): + logger.debug("Test %s: %d", "value", 42) + assert stderr.getvalue() == "[debug] Test value: 42\n" + + +def test_error_output(logger): + """Test error output.""" + stderr = StringIO() + with mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr): + logger.error("Error message") + assert stderr.getvalue() == "Error: Error message\n" + + +def test_error_with_formatting(logger): + """Test error output with string formatting.""" + stderr = StringIO() + with mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr): + logger.error("Error in %s: %s", "function", "details") + assert stderr.getvalue() == "Error: Error in function: details\n" + + +def test_multiple_messages(logger): + """Test multiple messages in sequence.""" + stderr = StringIO() + with ( + mock.patch.dict(os.environ, {"HELM_DEBUG": "1"}), + mock.patch("helm_values_manager.utils.logger.sys.stderr", stderr), + ): + logger.debug("Debug 1") + logger.error("Error 1") + logger.debug("Debug 2") + expected = "[debug] Debug 1\nError: Error 1\n[debug] Debug 2\n" + assert stderr.getvalue() == expected