From 195a9dd1d8e6a8f966418a59e717a961015d0b53 Mon Sep 17 00:00:00 2001 From: Rodolfo Olivieri Date: Wed, 22 May 2024 11:33:34 -0300 Subject: [PATCH] Apply suggestions from code review and few modifications In this commit, we are including the following modications that are worth mentioning: - We are dropping the filter for rhel repositories, so if the user has any rhel repositories enabled before the analysis, we should re-enable them after the analysis is done (alongside any other repository that rhsm reports that is enabled) - Updates to the integration tests to strip the white spaces and compare correctly the list of repositories enabled --- .../actions/pre_ponr_changes/subscription.py | 6 +++--- convert2rhel/backup/subscription.py | 15 +++++++++------ convert2rhel/subscription.py | 5 +---- .../unit_tests/backup/subscription_test.py | 19 ++++++++----------- ...est_enabled_repositories_after_analysis.py | 9 +++++---- 5 files changed, 26 insertions(+), 28 deletions(-) diff --git a/convert2rhel/actions/pre_ponr_changes/subscription.py b/convert2rhel/actions/pre_ponr_changes/subscription.py index ada5569b1a..2bdad2edb9 100644 --- a/convert2rhel/actions/pre_ponr_changes/subscription.py +++ b/convert2rhel/actions/pre_ponr_changes/subscription.py @@ -210,12 +210,12 @@ def run(self): restorable_subscription = RestorableSystemSubscription() backup.backup_control.push(restorable_subscription) + logger.task("Prepare: Subscription Manager - Disable all repositories") + backup.backup_control.push(RestorableDisableRepositories()) + logger.task("Prepare: Get RHEL repository IDs") rhel_repoids = repo.get_rhel_repoids() - logger.task("Prepare: Subscription Manager - Disable all repositories") - backup.backup_control.push(RestorableDisableRepositories(rhel_repos_ignore=rhel_repoids)) - # we need to enable repos after removing repofile pkgs, otherwise # we don't get backups to restore from on a rollback logger.task("Prepare: Subscription Manager - Enable RHEL repositories") diff --git a/convert2rhel/backup/subscription.py b/convert2rhel/backup/subscription.py index 1ac9b20cb9..6b496ae62b 100644 --- a/convert2rhel/backup/subscription.py +++ b/convert2rhel/backup/subscription.py @@ -91,10 +91,9 @@ class RestorableDisableRepositories(RestorableChange): # occurrences of the Repo ID in the output. ENABLED_REPOS_PATTERN = re.compile(r"Repo ID:\s+(?P\S+)") - def __init__(self, rhel_repos_ignore): + def __init__(self): super(RestorableDisableRepositories, self).__init__() self._repos_to_enable = [] - self._rhel_repos_ignore = rhel_repos_ignore or [] def _get_enabled_repositories(self): """Get repositories that were enabled prior to the conversion. @@ -109,9 +108,8 @@ def _get_enabled_repositories(self): repositories = [] matches = re.finditer(self.ENABLED_REPOS_PATTERN, output) if matches: - repositories = [ - match.group("repo_id") for match in matches if match.group("repo_id") not in self._rhel_repos_ignore - ] + repositories = [match.group("repo_id") for match in matches if match.group("repo_id")] + return repositories def enable(self): @@ -128,10 +126,15 @@ def restore(self): if not self.enabled: return - loggerinst.task("Rollback: Enabling RHSM repositories") + loggerinst.task("Rollback: Restoring state of the repositories") if self._repos_to_enable: loggerinst.debug("Repositories to enable: %s" % ",".join(self._repos_to_enable)) + + # This is not the ideal state. We should really have a generic + # class for enabling/disabling the repositories we have touched for + # RHSM. Jira issue: https://issues.redhat.com/browse/RHELC-1560 + subscription.disable_repos() subscription.submgr_enable_repos(self._repos_to_enable) super(RestorableDisableRepositories, self).restore() diff --git a/convert2rhel/subscription.py b/convert2rhel/subscription.py index 41a90b98ca..f5aca45ced 100644 --- a/convert2rhel/subscription.py +++ b/convert2rhel/subscription.py @@ -745,11 +745,8 @@ def disable_repos(): disabled. This can be overriden by the --disablerepo option. """ cmd = ["subscription-manager", "repos"] - disable_cmd = [] disabled_repos = ["*"] if not tool_opts.disablerepo else tool_opts.disablerepo - for repo in disabled_repos: - disable_cmd.append("--disable=%s" % repo) - + disable_cmd = ["".join("--disable=" + repo) for repo in disabled_repos] cmd.extend(disable_cmd) output, ret_code = utils.run_subprocess(cmd, print_output=False) if ret_code != 0: diff --git a/convert2rhel/unit_tests/backup/subscription_test.py b/convert2rhel/unit_tests/backup/subscription_test.py index 164df0745f..3f38a77391 100644 --- a/convert2rhel/unit_tests/backup/subscription_test.py +++ b/convert2rhel/unit_tests/backup/subscription_test.py @@ -149,7 +149,6 @@ class TestRestorableDisableRepositories: @pytest.mark.parametrize( ( "output", - "rhel_repos_ignore", "expected", ), ( @@ -168,7 +167,6 @@ class TestRestorableDisableRepositories: Repo URL: https://Satellite_Engineering_CentOS_7_Base.x86_64 Enabled: 1 """, - [], ["Test_Repository_ID", "Satellite_Engineering_CentOS_7_Base_x86_64"], ), ( @@ -186,8 +184,7 @@ class TestRestorableDisableRepositories: Repo URL: https://Satellite_Engineering_CentOS_7_Base.x86_64 Enabled: 1 """, - ["RHEL_Repository"], - ["Satellite_Engineering_CentOS_7_Base_x86_64"], + ["RHEL_Repository", "Satellite_Engineering_CentOS_7_Base_x86_64"], ), ( """ @@ -200,14 +197,13 @@ class TestRestorableDisableRepositories: Enabled: 1 """, ["RHEL_Repository"], - [], ), - ("There were no available repositories matching the specified criteria.", [], []), + ("There were no available repositories matching the specified criteria.", []), ), ) - def test_get_enabled_repositories(self, output, rhel_repos_ignore, expected, monkeypatch): + def test_get_enabled_repositories(self, output, expected, monkeypatch): monkeypatch.setattr(utils, "run_subprocess", mock.Mock(return_value=(output, 0))) - results = RestorableDisableRepositories(rhel_repos_ignore)._get_enabled_repositories() + results = RestorableDisableRepositories()._get_enabled_repositories() assert results == expected assert utils.run_subprocess.call_count == 1 @@ -233,7 +229,7 @@ def test_enable(self, enabled_repositories, log_message, monkeypatch, caplog): ) monkeypatch.setattr(subscription, "disable_repos", mock.Mock()) - action = RestorableDisableRepositories([]) + action = RestorableDisableRepositories() action.enable() assert action.enabled @@ -261,7 +257,8 @@ def test_enable(self, enabled_repositories, log_message, monkeypatch, caplog): ) def test_restore(self, enabled_repositories, log_message, monkeypatch, caplog): monkeypatch.setattr(subscription, "submgr_enable_repos", mock.Mock()) - action = RestorableDisableRepositories([]) + monkeypatch.setattr(subscription, "disable_repos", mock.Mock()) + action = RestorableDisableRepositories() action.enabled = True action._repos_to_enable = enabled_repositories @@ -274,7 +271,7 @@ def test_restore(self, enabled_repositories, log_message, monkeypatch, caplog): assert log_message % ",".join(enabled_repositories) in caplog.records[-1].message def test_not_enabled_restore(self): - action = RestorableDisableRepositories([]) + action = RestorableDisableRepositories() action.restore() assert not action.enabled diff --git a/tests/integration/tier0/non-destructive/enabled-repositories-after-analysis/test_enabled_repositories_after_analysis.py b/tests/integration/tier0/non-destructive/enabled-repositories-after-analysis/test_enabled_repositories_after_analysis.py index 71d7b0c763..7f9b2fd7a0 100644 --- a/tests/integration/tier0/non-destructive/enabled-repositories-after-analysis/test_enabled_repositories_after_analysis.py +++ b/tests/integration/tier0/non-destructive/enabled-repositories-after-analysis/test_enabled_repositories_after_analysis.py @@ -1,7 +1,5 @@ import pytest -from conftest import TEST_VARS - def collect_enabled_repositories(shell): """ @@ -18,7 +16,7 @@ def collect_enabled_repositories(shell): # Get the repo_id as in that split it will be the last thing in the # array. repo_id = line.split("Repo ID:")[-1] - enabled_repositories.append(repo_id) + enabled_repositories.append(repo_id.strip()) return enabled_repositories @@ -41,4 +39,7 @@ def test_enabled_repositories_after_analysis(shell, convert2rhel, satellite_regi enabled_repositories_after_analysis = collect_enabled_repositories(shell) - assert enabled_repositories_prior_analysis == enabled_repositories_after_analysis + # Repositories can be listed in a different order than the one we captured + # before the analysis. + for repository in enabled_repositories_after_analysis: + assert repository in enabled_repositories_prior_analysis