From f62c25d5fcb218c1668be1a7043ca45745c7c03b Mon Sep 17 00:00:00 2001 From: Kate Sugden <107400614+ksugden@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:46:50 +0100 Subject: [PATCH] fix: DBTP-1375 - platform helper version get-platform-helper-for-project no longer validates config (#575) --- dbt_platform_helper/COMMANDS.md | 4 +- dbt_platform_helper/commands/version.py | 58 +++++----- dbt_platform_helper/utils/platform_config.py | 27 +++-- dbt_platform_helper/utils/versioning.py | 46 ++++---- platform_helper.py | 4 +- tests/platform_helper/conftest.py | 47 ++++++++ tests/platform_helper/test_command_version.py | 100 +++++++++++------- .../utils/test_platform_config.py | 35 +++++- .../platform_helper/utils/test_versioning.py | 44 ++++++++ 9 files changed, 266 insertions(+), 99 deletions(-) diff --git a/dbt_platform_helper/COMMANDS.md b/dbt_platform_helper/COMMANDS.md index f7e23e70c..db08070b1 100644 --- a/dbt_platform_helper/COMMANDS.md +++ b/dbt_platform_helper/COMMANDS.md @@ -1002,8 +1002,8 @@ platform-helper database copy [↩ Parent](#platform-helper) - Contains subcommands for getting version information about the current - project. + Contains subcommands for getting version information about the + current project. ## Usage diff --git a/dbt_platform_helper/commands/version.py b/dbt_platform_helper/commands/version.py index 5dc20e6cd..5f82d8cfe 100644 --- a/dbt_platform_helper/commands/version.py +++ b/dbt_platform_helper/commands/version.py @@ -2,37 +2,37 @@ from dbt_platform_helper.utils.click import ClickDocOptGroup from dbt_platform_helper.utils.platform_config import get_environment_pipeline_names -from dbt_platform_helper.utils.versioning import ( - check_platform_helper_version_needs_update, -) from dbt_platform_helper.utils.versioning import get_required_platform_helper_version -@click.group(chain=True, cls=ClickDocOptGroup) -def version(): - """Contains subcommands for getting version information about the current - project.""" - check_platform_helper_version_needs_update() +class VersionCommand: + def __init__(self): + self.command_group = self.version + self.command = self.get_platform_helper_for_project + @click.group(chain=True, cls=ClickDocOptGroup) + def version(): + """Contains subcommands for getting version information about the + current project.""" -@version.command(help="Print the version of platform-tools required by the current project") -@click.option( - "--pipeline", - required=False, - type=click.Choice(get_environment_pipeline_names()), - help="Take into account platform-tools version overrides in the specified pipeline", -) -def get_platform_helper_for_project(pipeline): - """ - Version precedence is in this order: - - if the --pipeline option is supplied, the version in 'platform-config.yml' in: - environment_pipelines: - : - ... - versions: - platform-helper - - The version from default_versions/platform-helper in 'platform-config.yml' - - Fall back on the version in the deprecated '.platform-helper-version' file - """ - required_version = get_required_platform_helper_version(pipeline) - click.secho(required_version) + @version.command(help="Print the version of platform-tools required by the current project") + @click.option( + "--pipeline", + required=False, + type=click.Choice(get_environment_pipeline_names()), + help="Take into account platform-tools version overrides in the specified pipeline", + ) + def get_platform_helper_for_project(pipeline): + """ + Version precedence is in this order: + - if the --pipeline option is supplied, the version in 'platform-config.yml' in: + environment_pipelines: + : + ... + versions: + platform-helper + - The version from default_versions/platform-helper in 'platform-config.yml' + - Fall back on the version in the deprecated '.platform-helper-version' file + """ + required_version = get_required_platform_helper_version(pipeline) + click.secho(required_version) diff --git a/dbt_platform_helper/utils/platform_config.py b/dbt_platform_helper/utils/platform_config.py index 759cbe35c..ee2a57faf 100644 --- a/dbt_platform_helper/utils/platform_config.py +++ b/dbt_platform_helper/utils/platform_config.py @@ -1,17 +1,30 @@ -from pathlib import Path - import yaml +from pathlib import Path + from dbt_platform_helper.constants import PLATFORM_CONFIG_FILE -from dbt_platform_helper.utils.validation import load_and_validate_platform_config -def get_environment_pipeline_names(): - if not Path(PLATFORM_CONFIG_FILE).exists(): +def _read_config_file_contents(): + if Path(PLATFORM_CONFIG_FILE).exists(): + return Path(PLATFORM_CONFIG_FILE).read_text() + + +def load_unvalidated_config_file(): + file_contents = _read_config_file_contents() + if not file_contents: + return {} + try: + return yaml.safe_load(file_contents) + except yaml.parser.ParserError: return {} - config = load_and_validate_platform_config(disable_aws_validation=True, disable_file_check=True) - return config.get("environment_pipelines", {}).keys() + +def get_environment_pipeline_names(): + pipelines_config = load_unvalidated_config_file().get("environment_pipelines") + if pipelines_config: + return pipelines_config.keys() + return {} def is_terraform_project() -> bool: diff --git a/dbt_platform_helper/utils/versioning.py b/dbt_platform_helper/utils/versioning.py index 7fdb673db..5c87ee42e 100644 --- a/dbt_platform_helper/utils/versioning.py +++ b/dbt_platform_helper/utils/versioning.py @@ -16,7 +16,7 @@ from dbt_platform_helper.exceptions import IncompatibleMajorVersion from dbt_platform_helper.exceptions import IncompatibleMinorVersion from dbt_platform_helper.exceptions import ValidationException -from dbt_platform_helper.utils.validation import load_and_validate_platform_config +from dbt_platform_helper.utils.platform_config import load_unvalidated_config_file VersionTuple = Optional[Tuple[int, int, int]] @@ -104,22 +104,40 @@ def get_github_released_version(repository: str, tags: bool = False) -> Tuple[in return parse_version(package_info["tag_name"]) +def _get_latest_release(): + package_info = requests.get("https://pypi.org/pypi/dbt-platform-helper/json").json() + released_versions = package_info["releases"].keys() + parsed_released_versions = [parse_version(v) for v in released_versions] + parsed_released_versions.sort(reverse=True) + return parsed_released_versions[0] + + def get_platform_helper_versions(include_project_versions=True) -> PlatformHelperVersions: try: locally_installed_version = parse_version(version("dbt-platform-helper")) except PackageNotFoundError: locally_installed_version = None - package_info = requests.get("https://pypi.org/pypi/dbt-platform-helper/json").json() - released_versions = package_info["releases"].keys() - parsed_released_versions = [parse_version(v) for v in released_versions] - parsed_released_versions.sort(reverse=True) - latest_release = parsed_released_versions[0] + latest_release = _get_latest_release() + + if not include_project_versions: + return PlatformHelperVersions( + local_version=locally_installed_version, + latest_release=latest_release, + ) + + deprecated_version_file = Path(PLATFORM_HELPER_VERSION_FILE) + version_from_file = ( + parse_version(deprecated_version_file.read_text()) + if deprecated_version_file.exists() + else None + ) + + platform_config_default, pipeline_overrides = None, {} - platform_config_default, pipeline_overrides, version_from_file = None, {}, None + platform_config = load_unvalidated_config_file() - if include_project_versions: - platform_config = load_and_validate_platform_config(disable_aws_validation=True) + if platform_config: platform_config_default = parse_version( platform_config.get("default_versions", {}).get("platform-helper") ) @@ -130,13 +148,6 @@ def get_platform_helper_versions(include_project_versions=True) -> PlatformHelpe if pipeline.get("versions", {}).get("platform-helper") } - deprecated_version_file = Path(PLATFORM_HELPER_VERSION_FILE) - version_from_file = ( - parse_version(deprecated_version_file.read_text()) - if deprecated_version_file.exists() - else None - ) - out = PlatformHelperVersions( local_version=locally_installed_version, latest_release=latest_release, @@ -145,8 +156,7 @@ def get_platform_helper_versions(include_project_versions=True) -> PlatformHelpe pipeline_overrides=pipeline_overrides, ) - if include_project_versions: - _process_version_file_warnings(out) + _process_version_file_warnings(out) return out diff --git a/platform_helper.py b/platform_helper.py index 9821c42bf..076206324 100755 --- a/platform_helper.py +++ b/platform_helper.py @@ -20,7 +20,7 @@ from dbt_platform_helper.commands.notify import notify as notify_commands from dbt_platform_helper.commands.pipeline import pipeline as pipeline_commands from dbt_platform_helper.commands.secrets import secrets as secrets_commands -from dbt_platform_helper.commands.version import version as version_commands +from dbt_platform_helper.commands.version import VersionCommand from dbt_platform_helper.utils.click import ClickDocOptGroup @@ -47,7 +47,7 @@ def platform_helper(): platform_helper.add_command(secrets_commands) platform_helper.add_command(notify_commands) platform_helper.add_command(database_commands) -platform_helper.add_command(version_commands) +platform_helper.add_command(VersionCommand().command_group) if __name__ == "__main__": platform_helper() diff --git a/tests/platform_helper/conftest.py b/tests/platform_helper/conftest.py index fc709cedb..4759ca2a0 100644 --- a/tests/platform_helper/conftest.py +++ b/tests/platform_helper/conftest.py @@ -649,3 +649,50 @@ def s3_extensions_fixture(fakefs): } ), ) + + +INVALID_PLATFORM_CONFIG_WITH_PLATFORM_VERSION_OVERRIDES = """ +application: invalid-config-app +legacy_project: false + +default_versions: + platform-helper: 1.2.3 + terraform-platform-modules: 9.9.9 + +environments: + dev: + test: + staging: + prod: + vpc: prod-vpc + +extensions: + test-app-s3-bucket: + type: s3 + this_field_is_incompatible_with_current_version: foo + +environment_pipelines: + prod-main: + account: prod-acc + branch: main + slack_channel: "/codebuild/slack_oauth_channel" + trigger_on_push: false + versions: + platform-helper: 9.0.9 + environments: + prod: + requires_approval: true +""" + + +@pytest.fixture +def create_valid_platform_config_file(fakefs, valid_platform_config): + fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents=yaml.dump(valid_platform_config)) + + +@pytest.fixture +def create_invalid_platform_config_file(fakefs): + fakefs.create_file( + Path(PLATFORM_CONFIG_FILE), + contents=INVALID_PLATFORM_CONFIG_WITH_PLATFORM_VERSION_OVERRIDES, + ) diff --git a/tests/platform_helper/test_command_version.py b/tests/platform_helper/test_command_version.py index 9b7c65b78..2508a4d00 100644 --- a/tests/platform_helper/test_command_version.py +++ b/tests/platform_helper/test_command_version.py @@ -1,56 +1,76 @@ +import pytest import re -from pathlib import Path -from unittest.mock import patch -import yaml +from unittest.mock import patch from click.testing import CliRunner -from dbt_platform_helper.commands.version import get_platform_helper_for_project -from dbt_platform_helper.constants import PLATFORM_CONFIG_FILE +from dbt_platform_helper.commands.version import VersionCommand + + +@pytest.mark.usefixtures("create_valid_platform_config_file") +class TestVersionCommandWithValidConfig: + @patch("dbt_platform_helper.commands.version.get_required_platform_helper_version") + def test_calls_versioning_function_and_prints_returned_version( + self, + mock_get_required_platform_helper_version, + ): + mock_get_required_platform_helper_version.return_value = "1.2.3" + + command = VersionCommand().command + result = CliRunner().invoke(command, []) + + assert len(mock_get_required_platform_helper_version.mock_calls) == 1 + assert mock_get_required_platform_helper_version.mock_calls[0].args == (None,) + assert result.exit_code == 0 + assert re.match(r"\s*1\.2\.3\s*", result.output) + @patch("dbt_platform_helper.commands.version.get_required_platform_helper_version") + def test_calls_versioning_function_and_prints_returned_version_with_pipeline_override( + self, + mock_get_required_platform_helper_version, + ): + mock_get_required_platform_helper_version.return_value = "1.2.3" -@patch("dbt_platform_helper.commands.version.get_required_platform_helper_version") -def test_calls_versioning_function_and_prints_returned_version( - mock_get_required_platform_helper_version, - fakefs, - valid_platform_config, -): - fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents=yaml.dump(valid_platform_config)) - mock_get_required_platform_helper_version.return_value = "1.2.3" + command = VersionCommand().command + result = CliRunner().invoke(command, ["--pipeline", "main"]) - result = CliRunner().invoke(get_platform_helper_for_project, []) + assert len(mock_get_required_platform_helper_version.mock_calls) == 1 + assert mock_get_required_platform_helper_version.mock_calls[0].args == ("main",) + assert result.exit_code == 0 + assert re.match(r"\s*1\.2\.3\s*", result.output) - assert len(mock_get_required_platform_helper_version.mock_calls) == 1 - assert mock_get_required_platform_helper_version.mock_calls[0].args == (None,) - assert result.exit_code == 0 - assert re.match(r"\s*1\.2\.3\s*", result.output) + def test_fail_if_pipeline_option_is_not_a_pipeline(self): + command = VersionCommand().command + result = CliRunner().invoke(command, ["--pipeline", "bogus"]) + assert result.exit_code != 0 + assert "'bogus' is not one of" in result.output + assert "'main'" in result.output -@patch("dbt_platform_helper.commands.version.get_required_platform_helper_version") -def test_calls_versioning_function_and_prints_returned_version_with_pipeline_override( - mock_get_required_platform_helper_version, - fakefs, - valid_platform_config, -): - fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents=yaml.dump(valid_platform_config)) - mock_get_required_platform_helper_version.return_value = "1.2.3" - result = CliRunner().invoke(get_platform_helper_for_project, ["--pipeline", "main"]) +@pytest.mark.usefixtures("create_invalid_platform_config_file") +@patch("dbt_platform_helper.utils.versioning._get_latest_release", return_value="10.9.9") +class TestVersionCommandWithInvalidConfig: + def test_works_given_invalid_config(self, mock_latest_release): + command = VersionCommand().command + result = CliRunner().invoke(command, []) - assert len(mock_get_required_platform_helper_version.mock_calls) == 1 - assert mock_get_required_platform_helper_version.mock_calls[0].args == ("main",) - assert result.exit_code == 0 - assert re.match(r"\s*1\.2\.3\s*", result.output) + assert result.exit_code == 0 + assert result.output == "1.2.3\n" + def test_pipeline_override_given_invalid_config(self, mock_latest_release): + command = VersionCommand().command + result = CliRunner().invoke(command, ["--pipeline", "prod-main"]) -def test_fail_if_pipeline_option_is_not_a_pipeline( - fakefs, - valid_platform_config, -): - fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents=yaml.dump(valid_platform_config)) + assert result.exit_code == 0 + assert result.output == "9.0.9\n" - result = CliRunner().invoke(get_platform_helper_for_project, ["--pipeline", "bogus"]) + def test_fails_if_pipeline_option_is_not_a_pipeline_given_invalid_config( + self, mock_latest_release + ): + command = VersionCommand().command + result = CliRunner().invoke(command, ["--pipeline", "bogus"]) - assert result.exit_code != 0 - assert "'bogus' is not one of" in result.output - assert "'main'" in result.output + assert result.exit_code != 0 + assert "'bogus' is not " in result.output + assert "'prod-main'" in result.output diff --git a/tests/platform_helper/utils/test_platform_config.py b/tests/platform_helper/utils/test_platform_config.py index 9ad2df724..0e1b5513c 100644 --- a/tests/platform_helper/utils/test_platform_config.py +++ b/tests/platform_helper/utils/test_platform_config.py @@ -6,6 +6,7 @@ from dbt_platform_helper.constants import PLATFORM_CONFIG_FILE from dbt_platform_helper.utils.platform_config import get_environment_pipeline_names from dbt_platform_helper.utils.platform_config import is_terraform_project +from dbt_platform_helper.utils.platform_config import load_unvalidated_config_file def test_get_environment_pipeline_names(fakefs, valid_platform_config): @@ -16,7 +17,7 @@ def test_get_environment_pipeline_names(fakefs, valid_platform_config): assert {"main", "test", "prod-main"} == names -def test_get_environment_pipeline_names_defaults_to_empty_list_when_theres_no_platform_config( +def test_get_environment_pipeline_names_defaults_to_empty_list_when_theres_no_platform_config_file( fakefs, ): names = get_environment_pipeline_names() @@ -36,3 +37,35 @@ def test_is_terraform_project(fakefs, platform_config_content, expected_result): fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents=platform_config_content) assert is_terraform_project() == expected_result + + +def test_load_unvalidated_config_file_returns_a_dict_given_valid_yaml(fakefs): + fakefs.create_file( + Path(PLATFORM_CONFIG_FILE), + contents=""" +test: + some_key: some_value +""", + ) + config = load_unvalidated_config_file() + + assert config == {"test": {"some_key": "some_value"}} + + +def test_load_unvalidated_config_file_returns_empty_dict_given_invalid_yaml(fakefs): + fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents="{") + config = load_unvalidated_config_file() + + assert config == {} + + +def test_get_environment_pipeline_names_returns_empty_dict_given_invalid_yaml(fakefs): + fakefs.create_file(Path(PLATFORM_CONFIG_FILE), contents="{") + names = get_environment_pipeline_names() + assert names == {} + + +def test_get_environment_pipeline_names_given_invalid_config(create_invalid_platform_config_file): + names = get_environment_pipeline_names() + + assert {"prod-main"} == names diff --git a/tests/platform_helper/utils/test_versioning.py b/tests/platform_helper/utils/test_versioning.py index 70eb0e778..261447e9d 100644 --- a/tests/platform_helper/utils/test_versioning.py +++ b/tests/platform_helper/utils/test_versioning.py @@ -278,6 +278,50 @@ def test_get_platform_helper_versions( assert not mock_aws.called # Ensure that an AWS session is not required for this function +@patch("requests.get") +@patch("dbt_platform_helper.utils.versioning.version") +def test_get_platform_helper_versions_with_invalid_yaml_in_platform_config( + mock_local_version, mock_latest_release_request, fakefs +): + mock_local_version.return_value = "1.1.1" + mock_latest_release_request.return_value.json.return_value = { + "releases": {"1.2.3": None, "2.3.4": None, "0.1.0": None} + } + fakefs.create_file(PLATFORM_HELPER_VERSION_FILE, contents="5.6.7") + fakefs.create_file(PLATFORM_CONFIG_FILE, contents="{") + + versions = get_platform_helper_versions() + + assert versions.local_version == (1, 1, 1) + assert versions.latest_release == (2, 3, 4) + assert versions.platform_helper_file_version == (5, 6, 7) + assert versions.platform_config_default == None + assert versions.pipeline_overrides == {} + + +@patch("requests.get") +@patch("dbt_platform_helper.utils.versioning.version") +def test_get_platform_helper_versions_with_invalid_config( + mock_version, + mock_get, + fakefs, + create_invalid_platform_config_file, +): + mock_version.return_value = "1.1.1" + mock_get.return_value.json.return_value = { + "releases": {"1.2.3": None, "2.3.4": None, "0.1.0": None} + } + fakefs.create_file(PLATFORM_HELPER_VERSION_FILE, contents="5.6.7") + + versions = get_platform_helper_versions() + + assert versions.local_version == (1, 1, 1) + assert versions.latest_release == (2, 3, 4) + assert versions.platform_helper_file_version == (5, 6, 7) + assert versions.platform_config_default == (1, 2, 3) + assert versions.pipeline_overrides == {"prod-main": "9.0.9"} + + @pytest.mark.parametrize( "version_in_phv_file, version_in_platform_config, expected_message, message_colour", (