This is an automated email from the ASF dual-hosted git repository. asorokoumov pushed a commit to branch fixup-0.7.0-rc3 in repository https://gitbox.apache.org/repos/asf/otava.git
commit e5e8aaf1e63e4d0549245fb6f3ef0298774b23cf Author: Alex Sorokoumov <[email protected]> AuthorDate: Thu Nov 27 20:33:36 2025 -0800 Revert "Fix multiple issues discovered via running examples (#95)" This reverts commit 5276f24de6f0853f8304374dd1b3772501863c3c. --- docs/GRAFANA.md | 2 +- docs/GRAPHITE.md | 2 +- docs/POSTGRESQL.md | 2 +- examples/postgresql/init-db/schema.sql | 12 +- otava/config.py | 26 +--- pyproject.toml | 1 - tests/config_test.py | 232 +-------------------------------- uv.lock | 13 +- 8 files changed, 13 insertions(+), 277 deletions(-) diff --git a/docs/GRAFANA.md b/docs/GRAFANA.md index 922c0ad..13e8210 100644 --- a/docs/GRAFANA.md +++ b/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 analyze my-product.test --since=-10m --update-grafana +docker-compose -f examples/graphite/docker-compose.yaml run otava otava analyze my-product.test --since=-10m --update-grafana ``` Expected output: diff --git a/docs/GRAPHITE.md b/docs/GRAPHITE.md index 54d0e83..d7987b9 100644 --- a/docs/GRAPHITE.md +++ b/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 analyze my-product.test --since=-10m +docker-compose -f examples/graphite/docker-compose.yaml run otava otava analyze my-product.test --since=-10m ``` Expected output: diff --git a/docs/POSTGRESQL.md b/docs/POSTGRESQL.md index 7ecda0b..115483a 100644 --- a/docs/POSTGRESQL.md +++ b/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 analyze aggregate_mem --update-postgres +docker-compose -f examples/postgresql/docker-compose.yaml run --build otava bin/otava analyze aggregate_mem --update-postgres ``` Expected output: diff --git a/examples/postgresql/init-db/schema.sql b/examples/postgresql/init-db/schema.sql index 1092331..632cf3f 100644 --- a/examples/postgresql/init-db/schema.sql +++ b/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', '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'); + ('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'); INSERT INTO results (experiment_id, config_id, process_cumulative_rate_mean, process_cumulative_rate_stderr, process_cumulative_rate_diff) diff --git a/otava/config.py b/otava/config.py index ca1d587..345dede 100644 --- a/otava/config.py +++ b/otava/config.py @@ -20,7 +20,6 @@ from pathlib import Path from typing import Dict, List, Optional import configargparse -from expandvars import expandvars from ruamel.yaml import YAML from otava.bigquery import BigQueryConfig @@ -61,7 +60,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] @@ -81,7 +80,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") @@ -96,33 +95,12 @@ 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) diff --git a/pyproject.toml b/pyproject.toml index 6bb1409..24e3b36 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -48,7 +48,6 @@ dependencies = [ "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 4f66b95..5ce500e 100644 --- a/tests/config_test.py +++ b/tests/config_test.py @@ -15,19 +15,11 @@ # 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, - create_config_parser, - expand_env_vars_recursive, - load_config_from_file, - load_config_from_parser_args, -) +from otava.config import NestedYAMLConfigFileParser, load_config_from_file from otava.test_config import CsvTestConfig, GraphiteTestConfig, HistoStatTestConfig @@ -217,225 +209,3 @@ templates: 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 f2b2833..a5beddf 100644 --- a/uv.lock +++ b/uv.lock @@ -10,12 +10,11 @@ resolution-markers = [ [[package]] name = "apache-otava" -version = "0.7.0" +version = "0.6.1" 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" }, @@ -56,7 +55,6 @@ requires-dist = [ { 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" }, @@ -263,15 +261,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/02/cc/b7e31358aac6ed1ef2bb790a9746ac2c69bcb3c8588b41616914eb106eaf/exceptiongroup-1.2.2-py3-none-any.whl";, hash = "sha256:3111b9d131c238bec2f8f516e123e14ba243563fb135d3fe885990585aa7795b", size = 16453, upload-time = "2024-07-12T22:25:58.476Z" }, ] -[[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"
