Add Home Assistant secrets.yaml support for API keys and passwords #3128
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements Home Assistant's secrets mechanism in Predbat, allowing users to store sensitive configuration values (API keys, passwords, tokens) in a separate secrets.yaml file and reference them using the !secret tag in apps.yaml. The implementation is backward compatible and works seamlessly without a secrets file.
Key changes:
- Added
load_secrets()andsecret_constructor()methods to enable YAML!secrettag processing - Created comprehensive test suite covering various scenarios including missing files and missing keys
- Updated documentation with usage examples and recommendations for securing sensitive config values
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/predbat/hass.py | Core implementation: secrets loading from multiple locations and YAML constructor for !secret tag resolution |
| apps/predbat/tests/test_secrets.py | New test suite with 5 test cases covering secrets loading, tag resolution, and error handling |
| docs/apps-yaml.md | Added comprehensive "Storing secrets" section with usage guide and updated config recommendations |
| apps/predbat/config/secrets.yaml | Template file with examples of common secrets (API keys, passwords) and usage instructions |
| apps/predbat/config/apps.yaml | Updated template to demonstrate !secret tag usage for sensitive config values |
| coverage/standalone_ha | Added secrets.yaml copy for standalone testing |
| apps/predbat/unit_test.py | Registered new secrets test suite |
| apps/predbat/predbat.py | Version bump to v8.31.0 |
Comments suppressed due to low confidence (1)
apps/predbat/tests/test_secrets.py:119
- The test cleanup is missing in exception paths. If an assertion fails before the cleanup code runs (del os.environ, os.remove), the environment variable and test files will remain, potentially affecting subsequent tests.
Consider wrapping each test section in try/finally blocks or using context managers to ensure cleanup happens even when assertions fail. For example:
os.environ["PREDBAT_APPS_FILE"] = "test_apps.yaml"
try:
h = Hass()
assert h.args.get("api_key") == "secret_value_789", ...
finally:
if "PREDBAT_APPS_FILE" in os.environ:
del os.environ["PREDBAT_APPS_FILE"]
if os.path.exists("test_apps.yaml"):
os.remove("test_apps.yaml")
if os.path.exists("secrets.yaml"):
os.remove("secrets.yaml")This pattern should be applied to Test 4 (lines 89-97) and Test 5 (lines 113-119).
yaml.dump(secrets_data, f)
with open("test_apps.yaml", "w") as f:
f.write("pred_bat:\n")
f.write(" module: predbat\n")
f.write(" class: PredBat\n")
f.write(" missing_key: !secret non_existent_key\n")
os.environ["PREDBAT_APPS_FILE"] = "test_apps.yaml"
h = Hass()
assert h.args.get("missing_key") is None, f"Expected None for missing secret, got {h.args.get('missing_key')}"
print(" PASS - Missing secret key returns None and warns correctly")
del os.environ["PREDBAT_APPS_FILE"]
os.remove("test_apps.yaml")
os.remove("secrets.yaml")
print("**** test_secrets_loading PASSED ****")
return False # False = success in Predbat test framework
def run_secrets_tests(my_predbat=None):
"""
Run all secrets tests
"""
return test_secrets_loading()
| assert h.secrets == secrets_data, f"Expected {secrets_data}, got {h.secrets}" | ||
| os.remove("secrets.yaml") | ||
| print(" PASS - Secrets loaded from current directory") | ||
|
|
||
| # Test 3: Secrets file from PREDBAT_SECRETS_FILE env var | ||
| print(" Test 3: Secrets file from PREDBAT_SECRETS_FILE") | ||
| with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: |
There was a problem hiding this comment.
Test 2 doesn't clean up the secrets.yaml file if the assertion fails. If the assertion on line 45 fails, line 46 (os.remove) won't execute, leaving secrets.yaml in place for subsequent tests.
Consider using a try/finally block:
with open("secrets.yaml", "w") as f:
yaml.dump(secrets_data, f)
try:
h = Hass()
assert h.secrets == secrets_data, f"Expected {secrets_data}, got {h.secrets}"
print(" PASS - Secrets loaded from current directory")
finally:
if os.path.exists("secrets.yaml"):
os.remove("secrets.yaml")| os.remove(temp_secrets_file) | ||
| print(" PASS - Secrets loaded from PREDBAT_SECRETS_FILE") | ||
|
|
||
| # Test 4: Test !secret tag in apps.yaml | ||
| print(" Test 4: Test !secret tag resolution") | ||
| secrets_data = {"test_api_key": "secret_value_789", "test_username": "secret_user"} |
There was a problem hiding this comment.
Test 3 doesn't clean up the environment variable and temp file if the assertion fails. If the assertion on line 57 fails, lines 58-59 won't execute, potentially affecting subsequent tests or leaving temporary files on the system.
Consider using a try/finally block:
os.environ["PREDBAT_SECRETS_FILE"] = temp_secrets_file
try:
h = Hass()
assert h.secrets == secrets_data, f"Expected {secrets_data}, got {h.secrets}"
print(" PASS - Secrets loaded from PREDBAT_SECRETS_FILE")
finally:
if "PREDBAT_SECRETS_FILE" in os.environ:
del os.environ["PREDBAT_SECRETS_FILE"]
if os.path.exists(temp_secrets_file):
os.remove(temp_secrets_file)| # pylint: disable=attribute-defined-outside-init | ||
|
|
||
| import os | ||
| import yaml |
There was a problem hiding this comment.
The 'io' module is imported but never used in this file. All file operations use the built-in 'open()' function directly. This unused import should be removed.
| Create a `secrets.yaml` file in one of these locations (checked in order, only the first one is read): | ||
|
|
||
| 1. Path specified in `PREDBAT_SECRETS_FILE` environment variable | ||
| 2. `secrets.yaml` in the same directory as your `apps.yaml` |
There was a problem hiding this comment.
The priority order documented here does not match the actual implementation in hass.py. The documentation states the order is:
- PREDBAT_SECRETS_FILE environment variable
- secrets.yaml in same directory as apps.yaml
- /config/secrets.yaml (standard Home Assistant location)
But the actual code implementation checks:
- PREDBAT_SECRETS_FILE environment variable ✓
- secrets.yaml (current directory) ✓
- /config/secrets.yaml ✓
The issue is subtle: the documentation says "same directory as your apps.yaml" but the code actually checks "secrets.yaml" in the current working directory (wherever the process was started from), not necessarily where apps.yaml is located. If apps.yaml is loaded from a custom path via PREDBAT_APPS_FILE, the secrets.yaml lookup won't be relative to that location.
The documentation should be updated to accurately reflect this behavior.
| 2. `secrets.yaml` in the same directory as your `apps.yaml` | |
| 2. `secrets.yaml` in the current working directory (where the Predbat process is started; this may be different from the directory containing `apps.yaml` when using `PREDBAT_APPS_FILE`) |
| if os.path.exists("secrets.yaml"): | ||
| os.remove("secrets.yaml") | ||
| if os.path.exists("/config/secrets.yaml"): | ||
| os.remove("/config/secrets.yaml") |
There was a problem hiding this comment.
Test 1 attempts to remove /config/secrets.yaml but doesn't verify the test has write permissions to that directory. If the directory doesn't exist or is not writable, the os.remove call could raise an exception. The test should either:
- Check if the directory exists and is writable before attempting removal
- Wrap the removal in a try/except block
- Or use a more controlled test environment that doesn't rely on system paths
This is particularly important since /config/ is a system-level path that may have restricted permissions depending on where tests are run.
| os.remove("/config/secrets.yaml") | |
| try: | |
| os.remove("/config/secrets.yaml") | |
| except OSError: | |
| # Ignore errors if /config is not writable or file cannot be removed | |
| pass |
| # Register custom YAML constructor for !secret tag | ||
| yaml.add_constructor("!secret", self.secret_constructor, Loader=yaml.SafeLoader) |
There was a problem hiding this comment.
The yaml.add_constructor is being called globally for yaml.SafeLoader in the init method. This modifies a global YAML loader state and could cause issues if multiple Hass instances are created (as happens in the test suite). Each new instance will re-register the constructor, potentially causing side effects.
While this may work in practice since the constructor does the same thing, it's better practice to either:
- Only add the constructor once using a class-level flag to check if it's already registered
- Use a custom loader subclass instead of modifying the global SafeLoader
This is particularly important given that test_secrets.py creates 5 separate Hass() instances, each re-registering the global constructor.
| # Register custom YAML constructor for !secret tag | |
| yaml.add_constructor("!secret", self.secret_constructor, Loader=yaml.SafeLoader) | |
| # Register custom YAML constructor for !secret tag only once per class | |
| if not getattr(self.__class__, "_secret_constructor_registered", False): | |
| yaml.add_constructor("!secret", self.secret_constructor, Loader=yaml.SafeLoader) | |
| self.__class__._secret_constructor_registered = True |
| f.write("pred_bat:\n") | ||
| f.write(" module: predbat\n") | ||
| f.write(" class: PredBat\n") | ||
| f.write(" api_key: !secret test_api_key\n") |
There was a problem hiding this comment.
Variable test_config is not used.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Fixes #1864, raised over a year ago! |
Summary
Implements the Home Assistant secrets mechanism in Predbat, allowing users to store sensitive configuration values like API keys and passwords in a separate
secrets.yamlfile and reference them using the!secrettag inapps.yaml.Changes
Core Implementation
load_secrets()method inhass.pythat loadssecrets.yamlfrom:PREDBAT_SECRETS_FILEenvironment variablesecrets.yamlin the same directory asapps.yaml/config/secrets.yaml(standard Home Assistant location)secret_constructor()to handle!secrettagsapps.yamlfor seamless integrationNonewith a warning rather than raising an errorDocumentation
docs/apps-yaml.mdha_key- Home Assistant long-lived access tokenge_cloud_key- GivEnergy Cloud API keysolcast_api_key- Solcast solar forecast API keyoctopus_api_key- Octopus Energy API keyaxle_api_key- Axle Energy VPP API keyohme_password- Ohme charger passwordTemplate & Testing
templates/secrets.yamlwith common API key examplestests/test_secrets.pycovering:!secrettag resolutionBenefits
apps.yamlsafer to share for troubleshootingUsage Example
secrets.yaml:
apps.yaml:
Testing
secrets.yaml