Skip to content

Commit c547cb6

Browse files
committed
refactor: update PathData validation and ADR status
- Move required value validation into PathData.validate() loop - Add environment context to validation error messages - Add tests for PathData serialization methods - Mark ADR-005 (Unified Backend Approach) as Accepted - Mark ADR-007 (Sensitive Value Storage) as Accepted - Fix ADR-007 number in title - Update README.md to reflect ADR status changes This change improves error messages by including environment context and keeps validation close to the data it validates. PathData now has 100% test coverage.
1 parent b8a29a9 commit c547cb6

File tree

5 files changed

+110
-14
lines changed

5 files changed

+110
-14
lines changed

docs/ADRs/005-unified-backend-approach.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# ADR-005: Unified Backend Approach for Value Storage
22

33
## Status
4-
Proposed
4+
Accepted
55

66
## Context
77
Currently, the Value class handles two distinct storage types: local and remote. This creates a split in logic within the Value class, requiring different code paths and validation rules based on the storage type. This complexity makes the code harder to maintain and test.

docs/ADRs/007-sensitive-value-storage.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
# ADR 0002: Sensitive Value Storage
1+
# ADR 007: Sensitive Value Storage
22

33
## Status
4-
Proposed
4+
Accepted
55

66
## Context
77
The helm-values-manager needs to handle both sensitive and non-sensitive configuration values. While non-sensitive values can be stored directly in the configuration files, sensitive values require special handling to ensure security.

docs/ADRs/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ An Architecture Decision Record (ADR) is a document that captures an important a
3535
- **Dependencies**: ADR-003
3636

3737
### [ADR-005: Unified Backend Approach](005-unified-backend-approach.md)
38-
- **Status**: Proposed
38+
- **Status**: Accepted
3939
- **Context**: Split logic in Value class for different storage types
4040
- **Decision**: Remove storage type distinction, use SimpleValueBackend
4141
- **Impact**: Simplifies Value class and unifies storage interface
@@ -49,7 +49,7 @@ An Architecture Decision Record (ADR) is a document that captures an important a
4949
- **Dependencies**: ADR-001
5050

5151
### [ADR-007: Sensitive Value Storage](007-sensitive-value-storage.md)
52-
- **Status**: Proposed
52+
- **Status**: Accepted
5353
- **Context**: Need for secure handling of sensitive configuration values
5454
- **Decision**: Store sensitive values using reference-based approach in secure backends
5555
- **Impact**: Ensures security while maintaining flexibility and traceability

helm_values_manager/models/path_data.py

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,24 +31,27 @@ def validate(self) -> None:
3131
Validate the PathData instance.
3232
3333
Ensures that:
34-
1. If path is marked as required, all environments have values
35-
2. All Value instances use the same path as this PathData
34+
1. All Value instances use the same path as this PathData
35+
2. If path is marked as required, each environment has a non-empty value
3636
3737
Raises:
3838
ValueError: If validation fails
3939
"""
4040
logger.debug("Validating PathData for path: %s", self.path)
4141

42-
# Validate path consistency
42+
# Validate path consistency and required values
4343
for env, value in self._values.items():
44+
# Check path consistency
4445
if value.path != self.path:
4546
logger.error("Path mismatch for environment %s: %s != %s", env, value.path, self.path)
46-
raise ValueError(f"Value path {value.path} doesn't match PathData path {self.path}")
47-
48-
# If path is required, ensure all environments have values
49-
if self.metadata.get("required", False) and not self._values:
50-
logger.error("Missing required value for path %s", self.path)
51-
raise ValueError("Missing required value")
47+
raise ValueError(f"Value for environment {env} has inconsistent path: {value.path} != {self.path}")
48+
49+
# Check required values
50+
if self.metadata.get("required", False):
51+
val = value.get()
52+
if val is None or val == "":
53+
logger.error("Missing required value for path %s in environment %s", self.path, env)
54+
raise ValueError(f"Missing required value for path {self.path} in environment {env}")
5255

5356
def set_value(self, environment: str, value: Value) -> None:
5457
"""

tests/unit/models/test_path_data.py

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,3 +63,96 @@ def test_get_environments(path_data, mock_value):
6363
assert len(environments) == 2
6464
assert "test1" in environments
6565
assert "test2" in environments
66+
67+
68+
def test_validate_path_mismatch(path_data):
69+
"""Test validation with mismatched paths."""
70+
mock_value = Mock(spec=Value)
71+
mock_value.path = "wrong.path"
72+
path_data.set_value("test_env", mock_value)
73+
74+
with pytest.raises(
75+
ValueError, match=r"Value for environment test_env has inconsistent path: wrong\.path != test\.path"
76+
):
77+
path_data.validate()
78+
79+
80+
def test_validate_required_missing_value(path_data):
81+
"""Test validation with missing required value."""
82+
mock_value = Mock(spec=Value)
83+
mock_value.path = "test.path"
84+
mock_value.get.return_value = None
85+
path_data.set_value("test_env", mock_value)
86+
87+
with pytest.raises(ValueError, match=r"Missing required value for path test\.path in environment test_env"):
88+
path_data.validate()
89+
90+
91+
def test_validate_required_empty_value(path_data):
92+
"""Test validation with empty required value."""
93+
mock_value = Mock(spec=Value)
94+
mock_value.path = "test.path"
95+
mock_value.get.return_value = ""
96+
path_data.set_value("test_env", mock_value)
97+
98+
with pytest.raises(ValueError, match=r"Missing required value for path test\.path in environment test_env"):
99+
path_data.validate()
100+
101+
102+
def test_validate_success(path_data, mock_value):
103+
"""Test successful validation."""
104+
mock_value.get.return_value = "test_value"
105+
path_data.set_value("test_env", mock_value)
106+
path_data.validate() # Should not raise any error
107+
108+
109+
def test_validate_not_required(path_data, mock_value):
110+
"""Test validation when path is not required."""
111+
path_data.metadata["required"] = False
112+
mock_value.get.return_value = None
113+
path_data.set_value("test_env", mock_value)
114+
path_data.validate() # Should not raise any error
115+
116+
117+
def test_to_dict(path_data, mock_value):
118+
"""Test converting PathData to dictionary."""
119+
mock_value.to_dict.return_value = {"value": "test_value"}
120+
path_data.set_value("test_env", mock_value)
121+
122+
result = path_data.to_dict()
123+
assert result["path"] == "test.path"
124+
assert result["metadata"] == {
125+
"description": "Test description",
126+
"required": True,
127+
"sensitive": False,
128+
}
129+
assert result["values"] == {"test_env": {"value": "test_value"}}
130+
131+
132+
def test_from_dict():
133+
"""Test creating PathData from dictionary."""
134+
data = {
135+
"path": "test.path",
136+
"metadata": {
137+
"description": "Test description",
138+
"required": True,
139+
"sensitive": False,
140+
},
141+
"values": {"test_env": {"value": "test_value"}},
142+
}
143+
144+
def create_value_fn(path, env, value_data):
145+
mock_value = Mock(spec=Value)
146+
mock_value.path = path
147+
mock_value.environment = env
148+
return mock_value
149+
150+
path_data = PathData.from_dict(data, create_value_fn)
151+
assert path_data.path == "test.path"
152+
assert path_data.metadata == {
153+
"description": "Test description",
154+
"required": True,
155+
"sensitive": False,
156+
}
157+
assert len(list(path_data.get_environments())) == 1
158+
assert "test_env" in path_data._values

0 commit comments

Comments
 (0)