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

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")

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<repo_id>\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()

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:

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

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