Skip to content

Commit

Permalink
Update script to parse correctly None values (#105)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@r0x0d
r0x0d authored Jul 4, 2024
1 parent 7c75a7f commit e8d8d8e
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 54 deletions.
19 changes: 16 additions & 3 deletions convert2rhel_insights_tasks/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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
Expand Down
19 changes: 16 additions & 3 deletions playbooks/convert-to-rhel-analysis.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
19 changes: 16 additions & 3 deletions playbooks/convert-to-rhel-conversion.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
29 changes: 22 additions & 7 deletions tests/test_environment_variables.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
{},
),
),
)
Expand Down
63 changes: 25 additions & 38 deletions tests/test_run.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -114,6 +85,7 @@ def test_run_convert2rhel_els_option(
),
(
True,
"False",
"rhel-7-server-rpm, rhel-7-server-rpm-extras",
[
"/usr/bin/convert2rhel",
Expand All @@ -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",
Expand All @@ -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)
Expand Down

0 comments on commit e8d8d8e

Please sign in to comment.