Apply suggestions from code review

ansible/playbooks/roles/repository/files/download-requirements/src/command/dnf.py

@@ -1,21 +1,29 @@
 from typing import Dict, List
-import re
 
 from src.command.command import Command
 from src.error import CriticalError
 
 
-def filter_non_critical_errors(stderr: str):
-    re.sub('^Failed to set locale, defaulting to .+\n', '', stderr)
-
-class Dnf(Command):
+class DnfBase(Command):
     """
-    Interface for `dnf`
+    Base class for `dnf` interfaces
     """
 
     def __init__(self, retries: int):
         super().__init__('dnf', retries)
 
+    def _filter_non_critical_errors(self, stderr: str) -> str:  # pylint: disable=no-self-use
+        output_lines = [line for line in stderr.split(
+            '\n') if not line.startswith('Failed to set locale, defaulting to')]
+
+        return '\n'.join(output_lines)
+
+
+class Dnf(DnfBase):
+    """
+    Interface for `dnf`
+    """
+
     def update(self, package: str = None,
                      disablerepo: str = None,
                      enablerepo: str = None,
@@ -53,7 +61,7 @@ def update(self, package: str = None,
         if 'error' in proc.stdout:
             raise CriticalError(
                 f'Found an error. dnf update failed for package `{package}`, reason: `{proc.stdout}`')
-        if filter_non_critical_errors(proc.stderr):
+        if self._filter_non_critical_errors(proc.stderr):
             raise CriticalError(
                 f'dnf update failed for packages `{package}`, reason: `{proc.stderr}`')
 
@@ -76,7 +84,7 @@ def install(self, package: str,
         if 'error' in proc.stdout:
             raise CriticalError(
                 f'Found an error. dnf install failed for package `{package}`, reason: `{proc.stdout}`')
-        if filter_non_critical_errors(proc.stderr):
+        if self._filter_non_critical_errors(proc.stderr):
             raise CriticalError(
                 f'dnf install failed for package `{package}`, reason: `{proc.stderr}`')
 
@@ -114,6 +122,11 @@ def is_repo_enabled(self, repo: str) -> bool:
 
         return False
 
+    def are_all_repos_enabled(self, repos: List[str]) -> bool:
+        enabled_repos = self.__get_repo_ids()
+
+        return all(i in enabled_repos for i in repos)
+
     def accept_keys(self):
         # to accept import of repo's GPG key (for repo_gpgcheck=1)
         self.run(['repolist', '-y'])

ansible/playbooks/roles/repository/files/download-requirements/src/command/dnf_download.py

@@ -1,19 +1,15 @@
 from pathlib import Path
 from typing import List
 
-from src.command.command import Command
-from src.command.dnf import filter_non_critical_errors
+from src.command.dnf import DnfBase
 from src.error import CriticalError
 
 
-class DnfDownload(Command):
+class DnfDownload(DnfBase):
     """
     Interface for `dnf download`
     """
 
-    def __init__(self, retries: int):
-        super().__init__('dnf', retries)
-
     def download_packages(self, packages: List[str],
                           archlist: List[str],
                           destdir: Path,
@@ -39,6 +35,6 @@ def download_packages(self, packages: List[str],
         if 'error' in process.stdout:
             raise CriticalError(
                 f'Found an error. dnf download failed for packages `{packages}`, reason: `{process.stdout}`')
-        if filter_non_critical_errors(process.stderr):
+        if self._filter_non_critical_errors(process.stderr):
             raise CriticalError(
                 f'dnf download failed for packages `{packages}`, reason: `{process.stderr}`')

ansible/playbooks/roles/repository/files/download-requirements/src/mode/red_hat_family_mode.py

@@ -62,12 +62,8 @@ def _install_base_packages(self):
         # epel-release package is re-installed when repo it provides is not enabled
         epel_package_initially_present: bool = self._tools.rpm.is_package_installed('epel-release')
 
-        if (
-            epel_package_initially_present and
-            (not self._tools.dnf.is_repo_enabled('epel')
-             or not self._tools.dnf.is_repo_enabled('epel-modular'))
-        ):
-            self._tools.dnf.remove('epel-release')
+        if epel_package_initially_present and not self._tools.dnf.are_all_repos_enabled(['epel', 'epel-modular']):
+                self._tools.dnf.remove('epel-release')
 
         # some packages are from EPEL repo, ensure the latest version
         if not self._tools.rpm.is_package_installed('epel-release'):
@@ -128,21 +124,21 @@ def _add_third_party_repositories(self):
 
     def __remove_dnf_cache_for_custom_repos(self):
         # clean metadata for upgrades (when the same package can be downloaded from changed repo)
-        cache_paths: List[str] = list(self.__dnf_cache_dir.iterdir())
+        cache_paths: List[Path] = list(self.__dnf_cache_dir.iterdir())
 
-        id_names = [
+        def get_matched_paths(repo_id: str, paths: List[Path]) -> List[Path]:
+            return [path for path in paths if path.name.startswith(repo_id)]
+
+        repo_ids = [
             '2ndquadrant',
             'docker-ce',
             'epel',
-        ] + [repo['id'] for _, repo in self._repositories.items()]
+        ] + [repo['id'] for repo in self._repositories.values()]
 
-        matched_cache_paths: List[str] = []
+        matched_cache_paths: List[Path] = []
 
-        for path in cache_paths:
-            for repo_name in id_names:
-                if path.name.startswith(repo_name):
-                    matched_cache_paths.append(path)
-                    break
+        for repo_id in repo_ids:
+            matched_cache_paths.extend(get_matched_paths(repo_id, cache_paths))
 
         if matched_cache_paths:
             matched_cache_paths.sort()

ansible/playbooks/roles/repository/files/download-requirements/tests/command/test_dnf.py

@@ -1,7 +1,7 @@
-from tests.mocks.command_run_mock import CommandRunMock
-
 from src.command.dnf import Dnf
 
+from tests.mocks.command_run_mock import CommandRunMock
+
 
 def test_interface_update(mocker):
     ''' Check argument construction for `dnf update` '''

ansible/playbooks/roles/repository/files/download-requirements/tests/command/test_dnf_base.py

@@ -0,0 +1,12 @@
+from src.command.dnf import DnfBase
+
+
+def test_filter_non_critical_errors():
+    STDERR = '\n'.join([
+        '1st line',
+        'Failed to set locale, defaulting to C.UTF-8',
+        '3rd line'])
+
+    base = DnfBase(1)
+    output = base._filter_non_critical_errors(STDERR)
+    assert output == "1st line\n3rd line"