Expand all env variables and fix PostgreSQL example This commit fixes behavior removed in #86 - expand all env variables, not just ones matching CLI arguments. There are users relying on this behavior so we should not break it without longer discussion and a major release.
diff --git a/otava/config.py b/otava/config.py index 345dede..ca1d587 100644 --- a/otava/config.py +++ b/otava/config.py
@@ -20,6 +20,7 @@ from typing import Dict, List, Optional import configargparse +from expandvars import expandvars from ruamel.yaml import YAML from otava.bigquery import BigQueryConfig @@ -60,7 +61,7 @@ raise ConfigError("Property `tests` is not a dictionary") result = {} - for (test_name, test_config) in tests.items(): + for test_name, test_config in tests.items(): template_names = test_config.get("inherit", []) if not isinstance(template_names, List): template_names = [templates] @@ -80,7 +81,7 @@ raise ConfigError("Property `test_groups` is not a dictionary") result = {} - for (group_name, test_names) in groups.items(): + for group_name, test_names in groups.items(): test_list = [] if not isinstance(test_names, List): raise ConfigError(f"Test group {group_name} must be a list") @@ -95,12 +96,33 @@ return result +def expand_env_vars_recursive(obj): + """Recursively expand environment variables in all string values within a nested structure. + + Raises ConfigError if any environment variables remain undefined after expansion. + """ + if isinstance(obj, dict): + return {key: expand_env_vars_recursive(value) for key, value in obj.items()} + elif isinstance(obj, list): + return [expand_env_vars_recursive(item) for item in obj] + elif isinstance(obj, str): + return expandvars(obj, nounset=True) + else: + return obj + + def load_config_from_parser_args(args: configargparse.Namespace) -> Config: config_file = getattr(args, "config_file", None) if config_file is not None: yaml = YAML(typ="safe") config = yaml.load(Path(config_file).read_text()) + # Expand environment variables in the entire config after CLI argument replacement + try: + config = expand_env_vars_recursive(config) + except Exception as e: + raise ConfigError(f"Error expanding environment variables: {e}") + templates = load_templates(config) tests = load_tests(config, templates) groups = load_test_groups(config, tests)
diff --git a/pyproject.toml b/pyproject.toml index 24e3b36..6bb1409 100644 --- a/pyproject.toml +++ b/pyproject.toml
@@ -48,6 +48,7 @@ "google-cloud-bigquery>=3.25.0", "pg8000>=1.31.2", "configargparse>=1.7.1", + "expandvars>=0.12.0", ] [project.optional-dependencies]
diff --git a/tests/config_test.py b/tests/config_test.py index 5ce500e..4f66b95 100644 --- a/tests/config_test.py +++ b/tests/config_test.py
@@ -15,11 +15,19 @@ # specific language governing permissions and limitations # under the License. import os +import tempfile from io import StringIO import pytest +from expandvars import UnboundVariable -from otava.config import NestedYAMLConfigFileParser, load_config_from_file +from otava.config import ( + NestedYAMLConfigFileParser, + create_config_parser, + expand_env_vars_recursive, + load_config_from_file, + load_config_from_parser_args, +) from otava.test_config import CsvTestConfig, GraphiteTestConfig, HistoStatTestConfig @@ -209,3 +217,225 @@ for key in result.keys(): section = key.split('-')[0] assert section not in ignored_sections, f"Found key '{key}' from ignored section '{section}'" + + +def test_expand_env_vars_recursive(): + """Test the expand_env_vars_recursive function with various data types.""" + + # Set up test environment variables + test_env_vars = { + "TEST_HOST": "localhost", + "TEST_PORT": "8080", + "TEST_DB": "testdb", + "TEST_USER": "testuser", + } + + for key, value in test_env_vars.items(): + os.environ[key] = value + + try: + # Test simple string expansion + simple_string = "${TEST_HOST}:${TEST_PORT}" + result = expand_env_vars_recursive(simple_string) + assert result == "localhost:8080" + + # Test dictionary expansion + test_dict = { + "host": "${TEST_HOST}", + "port": "${TEST_PORT}", + "database": "${TEST_DB}", + "connection_string": "postgresql://${TEST_USER}@${TEST_HOST}:${TEST_PORT}/${TEST_DB}", + "timeout": 30, # non-string should remain unchanged + "enabled": True, # non-string should remain unchanged + } + + result_dict = expand_env_vars_recursive(test_dict) + expected_dict = { + "host": "localhost", + "port": "8080", + "database": "testdb", + "connection_string": "postgresql://testuser@localhost:8080/testdb", + "timeout": 30, + "enabled": True, + } + assert result_dict == expected_dict + + # Test list expansion + test_list = [ + "${TEST_HOST}", + {"nested_host": "${TEST_HOST}", "nested_port": "${TEST_PORT}"}, + ["${TEST_USER}", "${TEST_DB}"], + 123, # non-string should remain unchanged + ] + + result_list = expand_env_vars_recursive(test_list) + expected_list = [ + "localhost", + {"nested_host": "localhost", "nested_port": "8080"}, + ["testuser", "testdb"], + 123, + ] + assert result_list == expected_list + + # Test undefined variables (should throw UnboundVariable) + with pytest.raises(UnboundVariable, match="'UNDEFINED_VAR: unbound variable"): + expand_env_vars_recursive("${UNDEFINED_VAR}") + + # Test mixed defined/undefined variables (should throw UnboundVariable) + with pytest.raises(UnboundVariable, match="'UNDEFINED_VAR: unbound variable"): + expand_env_vars_recursive("prefix-${TEST_HOST}-middle-${UNDEFINED_VAR}-suffix") + + finally: + # Clean up environment variables + for key in test_env_vars: + if key in os.environ: + del os.environ[key] + + +def test_env_var_expansion_in_templates_and_tests(): + """Test that environment variable expansion works in template and test sections.""" + + # Set up test environment variables + test_env_vars = { + "CSV_DELIMITER": "$", + "CSV_QUOTE_CHAR": "!", + "CSV_FILENAME": "/tmp/test.csv", + } + + for key, value in test_env_vars.items(): + os.environ[key] = value + + # Create a temporary config file with env var placeholders + config_content = """ +templates: + csv_template_1: + csv_options: + delimiter: "${CSV_DELIMITER}" + + csv_template_2: + csv_options: + quote_char: '${CSV_QUOTE_CHAR}' + +tests: + expansion_test: + type: csv + file: ${CSV_FILENAME} + time_column: timestamp + metrics: + response_time: + column: response_ms + unit: ms + inherit: [csv_template_1, csv_template_2] +""" + + try: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: + f.write(config_content) + config_file_path = f.name + + try: + # Load config and verify expansion worked + parser = create_config_parser() + args = parser.parse_args(["--config-file", config_file_path]) + config = load_config_from_parser_args(args) + + # Verify test was loaded + assert "expansion_test" in config.tests + test = config.tests["expansion_test"] + assert isinstance(test, CsvTestConfig) + + # Verify that expansion worked + assert test.file == test_env_vars["CSV_FILENAME"] + + # Verify that inheritance from templates worked with expanded values + assert test.csv_options.delimiter == test_env_vars["CSV_DELIMITER"] + assert test.csv_options.quote_char == test_env_vars["CSV_QUOTE_CHAR"] + + finally: + os.unlink(config_file_path) + + finally: + # Clean up environment variables + for key in test_env_vars: + if key in os.environ: + del os.environ[key] + + +def test_cli_precedence_over_env_vars(): + """Test that CLI arguments take precedence over environment variables.""" + + # Set up environment variables + env_vars = { + "POSTGRES_HOSTNAME": "env-host.com", + "POSTGRES_PORT": "5433", + "POSTGRES_DATABASE": "env_db", + "SLACK_BOT_TOKEN": "env-slack-token", + } + + for key, value in env_vars.items(): + os.environ[key] = value + + # Create a simple config file + config_content = """ +postgres: + hostname: config_host + port: 5432 + database: config_db + username: config_user + password: config_pass + +slack: + token: config_slack_token +""" + + try: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: + f.write(config_content) + config_file_path = f.name + + try: + # Test 1: Only environment variables (no CLI overrides) + config_env_only = load_config_from_file(config_file_path) + + # Environment variables should override config file values + assert config_env_only.postgres.hostname == "env-host.com" + assert config_env_only.postgres.port == 5433 + assert config_env_only.postgres.database == "env_db" + assert config_env_only.slack.bot_token == "env-slack-token" + + # Values without env vars should use config file values + assert config_env_only.postgres.username == "config_user" + assert config_env_only.postgres.password == "config_pass" + + # Test 2: CLI arguments should override environment variables + cli_overrides = [ + "--postgres-hostname", + "cli-host.com", + "--postgres-port", + "5434", + "--slack-token", + "cli-slack-token", + ] + + config_cli_override = load_config_from_file( + config_file_path, arg_overrides=cli_overrides + ) + + # CLI overrides should win + assert config_cli_override.postgres.hostname == "cli-host.com" + assert config_cli_override.postgres.port == 5434 + assert config_cli_override.slack.bot_token == "cli-slack-token" + + # Values without CLI override should still use env vars + assert config_cli_override.postgres.database == "env_db" + assert config_cli_override.postgres.username == "config_user" + assert config_cli_override.postgres.password == "config_pass" + + finally: + os.unlink(config_file_path) + + finally: + # Clean up environment variables + for key in env_vars: + if key in os.environ: + del os.environ[key]
diff --git a/uv.lock b/uv.lock index a5beddf..f2b2833 100644 --- a/uv.lock +++ b/uv.lock
@@ -10,11 +10,12 @@ [[package]] name = "apache-otava" -version = "0.6.1" +version = "0.7.0" source = { editable = "." } dependencies = [ { name = "configargparse" }, { name = "dateparser" }, + { name = "expandvars" }, { name = "google-cloud-bigquery", version = "3.30.0", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version < '3.9'" }, { name = "google-cloud-bigquery", version = "3.35.1", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.9'" }, { name = "numpy" }, @@ -55,6 +56,7 @@ { name = "autoflake", marker = "extra == 'dev'", specifier = ">=1.4" }, { name = "configargparse", specifier = ">=1.7.1" }, { name = "dateparser", specifier = ">=1.0.0" }, + { name = "expandvars", specifier = ">=0.12.0" }, { name = "flake8", marker = "extra == 'dev'", specifier = ">=4.0.1" }, { name = "google-cloud-bigquery", specifier = ">=3.25.0" }, { name = "isort", marker = "extra == 'dev'", specifier = ">=5.10.1" }, @@ -262,6 +264,15 @@ ] [[package]] +name = "expandvars" +version = "1.1.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/9c/64/a9d8ea289d663a44b346203a24bf798507463db1e76679eaa72ee6de1c7a/expandvars-1.1.2.tar.gz", hash = "sha256:6c5822b7b756a99a356b915dd1267f52ab8a4efaa135963bd7f4bd5d368f71d7", size = 70842, upload-time = "2025-09-12T10:55:20.929Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/7f/e6/79c43f7a55264e479a9fbf21ddba6a73530b3ea8439a8bb7fa5a281721af/expandvars-1.1.2-py3-none-any.whl", hash = "sha256:d1652fe4e61914f5b88ada93aaedb396446f55ae4621de45c8cb9f66e5712526", size = 7526, upload-time = "2025-09-12T10:55:18.779Z" }, +] + +[[package]] name = "filelock" version = "3.16.1" source = { registry = "https://pypi.org/simple" }