From e8d8d8eaeb7ac2c3725ee2917767a9d70e733ffc Mon Sep 17 00:00:00 2001 From: Rodolfo Olivieri Date: Thu, 4 Jul 2024 13:48:29 -0300 Subject: [PATCH] Update script to parse correctly None values (#105) When we parse the values coming from environment variable, the `None` value is parsed as a string. This patch introduces a simple solution to deal with that and not append a wrong value to the command line switch and to the environment context. --- convert2rhel_insights_tasks/main.py | 19 +++++-- playbooks/convert-to-rhel-analysis.yml | 19 +++++-- playbooks/convert-to-rhel-conversion.yml | 19 +++++-- tests/test_environment_variables.py | 29 ++++++++--- tests/test_run.py | 63 ++++++++++-------------- 5 files changed, 95 insertions(+), 54 deletions(-) diff --git a/convert2rhel_insights_tasks/main.py b/convert2rhel_insights_tasks/main.py index 584496c..6a31b38 100644 --- a/convert2rhel_insights_tasks/main.py +++ b/convert2rhel_insights_tasks/main.py @@ -674,7 +674,13 @@ def prepare_environment_variables(env): :type env: dict[str, Any] """ for variable, value in env.items(): - if variable.startswith("CONVERT2RHEL_") and value == 0: + # We can pop out of context both the OPTIONAL_REPOSITORIES and + # ELS_DISABLED envs as they are not necessary for convert2rhel + # execution. + if variable in ("OPTIONAL_REPOSITORIES", "ELS_DISABLED"): + env.pop(variable) + + if variable.startswith("CONVERT2RHEL_") and value == "0": env.pop(variable) return env @@ -701,8 +707,15 @@ def run_convert2rhel(env): if not bool(els_disabled): command.append("--els") - optional_repositories = env.pop("OPTIONAL_REPOSITORIES", []) - if optional_repositories: + optional_repositories = env.pop("OPTIONAL_REPOSITORIES", None) + # The `None` value that comes from the playbook gets converted to "None" + # when we parse it from the environment variable, to not mess with casting + # and converting, the easiest option is to check against that value for + # now. + # TODO(r0x0d): The ideal solution here would be coming with a pre-defined + # dictionary of values that have the correct values and types. Maybe for + # the future. + if optional_repositories and optional_repositories != "None": repositories = optional_repositories.split(",") repositories = [ "--enablerepo=%s" % repository.strip() for repository in repositories diff --git a/playbooks/convert-to-rhel-analysis.yml b/playbooks/convert-to-rhel-analysis.yml index 467792d..ae371b2 100644 --- a/playbooks/convert-to-rhel-analysis.yml +++ b/playbooks/convert-to-rhel-analysis.yml @@ -683,7 +683,13 @@ :type env: dict[str, Any] """ for variable, value in env.items(): - if variable.startswith("CONVERT2RHEL_") and value == 0: + # We can pop out of context both the OPTIONAL_REPOSITORIES and + # ELS_DISABLED envs as they are not necessary for convert2rhel + # execution. + if variable in ("OPTIONAL_REPOSITORIES", "ELS_DISABLED"): + env.pop(variable) + + if variable.startswith("CONVERT2RHEL_") and value == "0": env.pop(variable) return env @@ -710,8 +716,15 @@ if not bool(els_disabled): command.append("--els") - optional_repositories = env.pop("OPTIONAL_REPOSITORIES", []) - if optional_repositories: + optional_repositories = env.pop("OPTIONAL_REPOSITORIES", None) + # The `None` value that comes from the playbook gets converted to "None" + # when we parse it from the environment variable, to not mess with casting + # and converting, the easiest option is to check against that value for + # now. + # TODO(r0x0d): The ideal solution here would be coming with a pre-defined + # dictionary of values that have the correct values and types. Maybe for + # the future. + if optional_repositories and optional_repositories != "None": repositories = optional_repositories.split(",") repositories = [ "--enablerepo=%s" % repository.strip() for repository in repositories diff --git a/playbooks/convert-to-rhel-conversion.yml b/playbooks/convert-to-rhel-conversion.yml index b32273c..c1c12d2 100644 --- a/playbooks/convert-to-rhel-conversion.yml +++ b/playbooks/convert-to-rhel-conversion.yml @@ -683,7 +683,13 @@ :type env: dict[str, Any] """ for variable, value in env.items(): - if variable.startswith("CONVERT2RHEL_") and value == 0: + # We can pop out of context both the OPTIONAL_REPOSITORIES and + # ELS_DISABLED envs as they are not necessary for convert2rhel + # execution. + if variable in ("OPTIONAL_REPOSITORIES", "ELS_DISABLED"): + env.pop(variable) + + if variable.startswith("CONVERT2RHEL_") and value == "0": env.pop(variable) return env @@ -710,8 +716,15 @@ if not bool(els_disabled): command.append("--els") - optional_repositories = env.pop("OPTIONAL_REPOSITORIES", []) - if optional_repositories: + optional_repositories = env.pop("OPTIONAL_REPOSITORIES", None) + # The `None` value that comes from the playbook gets converted to "None" + # when we parse it from the environment variable, to not mess with casting + # and converting, the easiest option is to check against that value for + # now. + # TODO(r0x0d): The ideal solution here would be coming with a pre-defined + # dictionary of values that have the correct values and types. Maybe for + # the future. + if optional_repositories and optional_repositories != "None": repositories = optional_repositories.split(",") repositories = [ "--enablerepo=%s" % repository.strip() for repository in repositories diff --git a/tests/test_environment_variables.py b/tests/test_environment_variables.py index cbe582c..c91ba85 100644 --- a/tests/test_environment_variables.py +++ b/tests/test_environment_variables.py @@ -43,28 +43,43 @@ def test_parse_environment_variables_empty(monkeypatch): {}, ), ( - {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": 0}, + {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": "0"}, {}, ), ( - {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": 1}, - {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": 1}, + {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": "1"}, + {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": "1"}, ), ( {"CONVERT2RHEL_CONFIGURE_HOST_METERING": "auto"}, {"CONVERT2RHEL_CONFIGURE_HOST_METERING": "auto"}, ), ( - {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": 0, "SCRIPT_MODE": "ANALYSIS"}, + {"CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": "0", "SCRIPT_MODE": "ANALYSIS"}, {"SCRIPT_MODE": "ANALYSIS"}, ), ( { - "CONVERT2RHEL_OUTDATED_PACKAGE_CHECK_SKIP": 1, - "CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": 0, + "CONVERT2RHEL_OUTDATED_PACKAGE_CHECK_SKIP": "1", + "CONVERT2RHEL_ALLOW_UNAVAILABLE_KMODS": "0", "SCRIPT_MODE": "ANALYSIS", }, - {"CONVERT2RHEL_OUTDATED_PACKAGE_CHECK_SKIP": 1, "SCRIPT_MODE": "ANALYSIS"}, + { + "CONVERT2RHEL_OUTDATED_PACKAGE_CHECK_SKIP": "1", + "SCRIPT_MODE": "ANALYSIS", + }, + ), + ( + {"OPTIONAL_REPOSITORIES": "None"}, + {}, + ), + ( + {"ELS_DISABLED": "True"}, + {}, + ), + ( + {"ELS_DISABLED": "True", "OPTIONAL_REPOSITORIES": "None"}, + {}, ), ), ) diff --git a/tests/test_run.py b/tests/test_run.py index c2642ae..481d82b 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -63,46 +63,17 @@ def test_run_convert2rhel_analysis(monkeypatch): ( "is_analysis", "els_disabled", - "expected_cmd", - ), - ( - (True, "True", ["/usr/bin/convert2rhel", "analyze", "-y"]), - (True, "true", ["/usr/bin/convert2rhel", "analyze", "-y"]), - (True, "False", ["/usr/bin/convert2rhel", "analyze", "-y", "--els"]), - (True, "false", ["/usr/bin/convert2rhel", "analyze", "-y", "--els"]), - # Make sure that this also pass for conversion - (False, "True", ["/usr/bin/convert2rhel", "-y"]), - (False, "true", ["/usr/bin/convert2rhel", "-y"]), - (False, "False", ["/usr/bin/convert2rhel", "-y", "--els"]), - (False, "false", ["/usr/bin/convert2rhel", "-y", "--els"]), - ), -) -def test_run_convert2rhel_els_option( - is_analysis, els_disabled, expected_cmd, monkeypatch -): - monkeypatch.setattr(main, "IS_ANALYSIS", is_analysis) - mock_env = {"PATH": "/fake/path", "ELS_DISABLED": els_disabled} - - with patch( - "convert2rhel_insights_tasks.main.run_subprocess", return_value=(b"", 0) - ) as mock_popen: - run_convert2rhel(mock_env) - - mock_popen.assert_called_once_with( - expected_cmd, - env={"PATH": "/fake/path"}, - ) - - -@pytest.mark.parametrize( - ( - "is_analysis", "optional_repositories", "expected_cmd", ), ( + (True, "True", "None", ["/usr/bin/convert2rhel", "analyze", "-y"]), + (True, "true", "None", ["/usr/bin/convert2rhel", "analyze", "-y"]), + (True, "False", "None", ["/usr/bin/convert2rhel", "analyze", "-y", "--els"]), + (True, "false", "None", ["/usr/bin/convert2rhel", "analyze", "-y", "--els"]), ( True, + "False", "rhel-7-server-rpm", [ "/usr/bin/convert2rhel", @@ -114,6 +85,7 @@ def test_run_convert2rhel_els_option( ), ( True, + "False", "rhel-7-server-rpm, rhel-7-server-rpm-extras", [ "/usr/bin/convert2rhel", @@ -125,13 +97,24 @@ def test_run_convert2rhel_els_option( ], ), # Make sure that this also pass for conversion + (False, "True", "None", ["/usr/bin/convert2rhel", "-y"]), + (False, "true", "None", ["/usr/bin/convert2rhel", "-y"]), + (False, "False", "None", ["/usr/bin/convert2rhel", "-y", "--els"]), + (False, "false", "None", ["/usr/bin/convert2rhel", "-y", "--els"]), ( False, + "False", "rhel-7-server-rpm", - ["/usr/bin/convert2rhel", "-y", "--els", "--enablerepo=rhel-7-server-rpm"], + [ + "/usr/bin/convert2rhel", + "-y", + "--els", + "--enablerepo=rhel-7-server-rpm", + ], ), ( False, + "False", "rhel-7-server-rpm, rhel-7-server-rpm-extras", [ "/usr/bin/convert2rhel", @@ -143,11 +126,15 @@ def test_run_convert2rhel_els_option( ), ), ) -def test_run_convert2rhel_optional_repositories( - is_analysis, optional_repositories, expected_cmd, monkeypatch +def test_run_convert2rhel_command_line_switch( + is_analysis, els_disabled, optional_repositories, expected_cmd, monkeypatch ): monkeypatch.setattr(main, "IS_ANALYSIS", is_analysis) - mock_env = {"PATH": "/fake/path", "OPTIONAL_REPOSITORIES": optional_repositories} + mock_env = { + "PATH": "/fake/path", + "ELS_DISABLED": els_disabled, + "OPTIONAL_REPOSITORIES": optional_repositories, + } with patch( "convert2rhel_insights_tasks.main.run_subprocess", return_value=(b"", 0)