Fix multiple issues discovered via running examples

docs/GRAFANA.md

@@ -61,7 +61,7 @@ docker-compose -f examples/graphite/docker-compose.yaml up --force-recreate --al
 Run otava in another tab:
 
 ```bash
-docker-compose -f examples/graphite/docker-compose.yaml run otava otava analyze my-product.test --since=-10m --update-grafana
+docker-compose -f examples/graphite/docker-compose.yaml run otava analyze my-product.test --since=-10m --update-grafana
 ```
 
 Expected output:

docs/GRAPHITE.md

@@ -105,7 +105,7 @@ docker-compose -f examples/graphite/docker-compose.yaml up --force-recreate --al
 Run otava in another tab:
 
 ```bash
-docker-compose -f examples/graphite/docker-compose.yaml run otava otava analyze my-product.test --since=-10m
+docker-compose -f examples/graphite/docker-compose.yaml run otava analyze my-product.test --since=-10m
 ```
 
 Expected output:

docs/POSTGRESQL.md

@@ -99,7 +99,7 @@ docker-compose -f examples/postgresql/docker-compose.yaml up --force-recreate --
 Run Otava in the other tab to show results for a single test `aggregate_mem` and update the database with newly found change points:
 
 ```bash
-docker-compose -f examples/postgresql/docker-compose.yaml run --build otava bin/otava analyze aggregate_mem --update-postgres
+docker-compose -f examples/postgresql/docker-compose.yaml run --build otava analyze aggregate_mem --update-postgres
 ```
 
 Expected output:

examples/postgresql/init-db/schema.sql

@@ -75,12 +75,12 @@ INSERT INTO configs (id, benchmark, store, instance_type, cache) VALUES
 INSERT INTO experiments
     (id, ts, branch, commit, commit_ts, username, details_url)
 VALUES
-    ('aggregate-36e5ccd2', '2024-03-14 12:03:02+00', 'trunk', '36e5ccd2', '2024-03-13 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-36e5ccd2'),
-    ('aggregate-d5460f38', '2024-03-27 12:03:02+00', 'trunk', 'd5460f38', '2024-03-25 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-d5460f38'),
-    ('aggregate-bc9425cb', '2024-04-01 12:03:02+00', 'trunk', 'bc9425cb', '2024-04-02 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-bc9425cb'),
-    ('aggregate-14df1b11', '2024-04-07 12:03:02+00', 'trunk', '14df1b11', '2024-04-06 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-14df1b11'),
-    ('aggregate-ac40c0d8', '2024-04-14 12:03:02+00', 'trunk', 'ac40c0d8', '2024-04-13 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-ac40c0d8'),
-    ('aggregate-0af4ccbc', '2024-04-28 12:03:02+00', 'trunk', '0af4ccbc', '2024-04-27 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-0af4ccbc');
+    ('aggregate-36e5ccd2', '2025-03-14 12:03:02+00', 'trunk', '36e5ccd2', '2025-03-13 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-36e5ccd2'),
+    ('aggregate-d5460f38', '2025-03-27 12:03:02+00', 'trunk', 'd5460f38', '2025-03-25 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-d5460f38'),
+    ('aggregate-bc9425cb', '2025-04-01 12:03:02+00', 'trunk', 'bc9425cb', '2025-04-02 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-bc9425cb'),
+    ('aggregate-14df1b11', '2025-04-07 12:03:02+00', 'trunk', '14df1b11', '2025-04-06 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-14df1b11'),
+    ('aggregate-ac40c0d8', '2025-04-14 12:03:02+00', 'trunk', 'ac40c0d8', '2025-04-13 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-ac40c0d8'),
+    ('aggregate-0af4ccbc', '2025-04-28 12:03:02+00', 'trunk', '0af4ccbc', '2025-04-27 10:03:02+00', 'ci', 'https://example.com/experiments/aggregate-0af4ccbc');
 
 
 INSERT INTO results (experiment_id, config_id, process_cumulative_rate_mean, process_cumulative_rate_stderr, process_cumulative_rate_diff)

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 @@ def load_tests(config: Dict, templates: Dict) -> Dict[str, TestConfig]:
         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 @@ def load_test_groups(config: Dict, tests: Dict[str, TestConfig]) -> Dict[str, Li
         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 @@ def load_test_groups(config: Dict, tests: Dict[str, TestConfig]) -> Dict[str, Li
     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)

pyproject.toml

@@ -48,6 +48,7 @@ dependencies = [
     "google-cloud-bigquery>=3.25.0",
     "pg8000>=1.31.2",
     "configargparse>=1.7.1",
+    "expandvars>=0.12.0",
 ]
 
 [project.optional-dependencies]

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 @@ def test_config_section_yaml_parser_flattens_only_config_sections():
     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]