Repository: cekit/cekit

Issue:
refactor template.jinja to fully abstract package manager specifics
Spinning this out of #886: something I've been thinking of a while, I think we should have all package manager specifics out of `template.jinja` and handled elsewhere, perhaps via `TemplateHelper`. For example, stuff like 

```
{%- macro pkg_install(pkg_manager, pkg_manager_flags, packages) -%}
    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
{{ pkg_manager }} {{ helper.package_manager_flags(pkg_manager, pkg_manager_flags) }} {{ helper.package_manager_install(pkg_manager) }} {%- for package in packages %} {{ package }}{% endfor %} \
    {% if pkg_manager == 'microdnf' %}
            && {{ pkg_manager }} clean all \
...
 ```

it's awkward to have both a set-test (`in SUPPORTED_PACKAGE_MANAGERS`) and branching logic on hard-coded specific values (`if pkg_manager == 'microdnf'`) and these macros are harder to read, write or test compared to normal Python code.

I think we should have some kind of abstract class encapsulating all package manager stuff with specific sub-classes for supported package managers, all in proper python code. Something like 

```
class PackageManager(object):
  def update_metadata(self): raise NotImplementedError(...)
  def install_packages(self, pkgs): raise NotImplementedError(...)
  ...
...
class Microdnf(PackageManager):
...
```

and the templates could simplify a lot down to something like `helper.get_manager().install_packages(packages)`.

This would also make adding new package managers much easier.

Discussion:
@rnc: I do agree the macros are harder to follow and make maintenance more complex. I have no objections to refactoring to make it easier to maintain/support.@rnc: As a slight aside, I'm not sure why we have checks like ` {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}` ; I don't think its possible to have a manager that is **not** one of the supported types defined in https://github.com/cekit/cekit/blob/develop/cekit/descriptor/packages.py#L33 @rnc: Having dug into this a bit it looks like it would be straightforward to remove the `SUPPORTED_PACKAGE_MANAGERS` and move/inline some of the other checks which would simplify the code quite a bit. This does _not_ factor out into separate classes - whether that would really be required after this simplification is another discussion.

Solution patch:
diff --git a/.github/workflows/cekit.yml b/.github/workflows/cekit.yml
index 55b90a5..14b4ed9 100644
--- a/.github/workflows/cekit.yml
+++ b/.github/workflows/cekit.yml
@@ -17,7 +17,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        python-version: [ '3.6', '3.7', '3.8', '3.9', '3.10', '3.11', '3.12' ]
+        python-version: [ '3.6', '3.7', '3.8', '3.9', '3.10', '3.11', '3.12', '3.13' ]
         os: [macos-13, ubuntu-20.04]
     runs-on: ${{ matrix.os }}
     steps:
diff --git a/cekit/template_helper.py b/cekit/template_helper.py
index 0cdecee..98b4f4f 100644
--- a/cekit/template_helper.py
+++ b/cekit/template_helper.py
@@ -2,8 +2,6 @@ import os
 
 
 class TemplateHelper(object):
-    SUPPORTED_PACKAGE_MANAGERS = ["yum", "dnf", "microdnf", "apk", "apt-get"]
-
     def __init__(self, module_registry):
         self._module_registry = module_registry
 
@@ -169,13 +167,26 @@ class TemplateHelper(object):
         else:
             return "reinstall -y"
 
-    def package_manager_query(self, pkg_mgr):
+    def package_manager_cleanup(self, image):
+        if (
+            self.packages_to_install(image)
+            or self.packages_to_remove(image)
+            or self.packages_to_reinstall(image)
+            and image.packages.manager in ["yum", "dnf", "microdnf"]
+        ):
+            return True
+
+    def package_manager_clean_and_query(self, pkg_mgr):
         if "apk" in pkg_mgr:
             return "apk info -e"
         elif "apt-get" in pkg_mgr:
             return "dpkg-query --list"
         else:
-            return "rpm -q"
+            result = ""
+            if "microdnf" in pkg_mgr:
+                result += "microdnf clean all && "
+            result += "rpm -q"
+            return result
 
     def package_manager_remove(self, pkg_mgr):
         if "apk" in pkg_mgr:
diff --git a/cekit/templates/template.jinja b/cekit/templates/template.jinja
index f2ad9b1..d166e7d 100644
--- a/cekit/templates/template.jinja
+++ b/cekit/templates/template.jinja
@@ -1,81 +1,34 @@
 {#
  # Creates a package repository definition from the system.
- #
- # Works only when the package manager is supported.
  #}
  {%- macro repo_create(pkg_manager, repositories) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
-        {% for repo in repositories %}
+    {% for repo in repositories %}
 repos/{{ repo.filename }} \
-        {% endfor %}
+    {% endfor %}
     /etc/yum.repos.d/
-    {% endif %}
-{%- endmacro -%}
-
-{#
- # Removes specified package repository from the system.
- #
- # Works only when the package manager is supported.
- #}
-{%- macro repo_remove(pkg_manager, repositories) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
-rm{% for repo in repositories %} /etc/yum.repos.d/{{ repo.filename }}{% endfor %}
-    {% endif %}
-{%- endmacro -%}
-
-{#
- # Installs a repository RPM using specified package manager.
- #
- # Works only when the package manager is supported.
- #}
-{%- macro repo_install(pkg_manager, pkg_manager_flags, rpm) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
-{{ pkg_manager }} {{ helper.package_manager_flags(pkg_manager, pkg_manager_flags) }} install -y {{ rpm }}
-    {% endif %}
 {%- endmacro -%}
 
 {#
  # Removes packages using specified package manager.
  #}
 {%- macro pkg_remove(pkg_manager, pkg_manager_flags, packages) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
 {{ pkg_manager }} {{ helper.package_manager_flags(pkg_manager, pkg_manager_flags) }} {{ helper.package_manager_remove(pkg_manager) }} {%- for package in packages %} {{ package }}{% endfor %}
-    {% endif %}
 {%- endmacro -%}
 
 {#
  # Installs packages using specified package manager.
  #}
 {%- macro pkg_install(pkg_manager, pkg_manager_flags, packages) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
 {{ pkg_manager }} {{ helper.package_manager_flags(pkg_manager, pkg_manager_flags) }} {{ helper.package_manager_install(pkg_manager) }} {%- for package in packages %} {{ package }}{% endfor %} \
-    {% if pkg_manager == 'microdnf' %}
-            && {{ pkg_manager }} clean all \
-    {% endif %}
-            && {{ helper.package_manager_query(pkg_manager) }}{% for package in packages %} {{ package.split('=')[0] }}{% endfor %}
-    {% endif %}
+            && {{ helper.package_manager_clean_and_query(pkg_manager) }}{% for package in packages %} {{ package.split('=')[0] }}{% endfor %}
 {%- endmacro -%}
 
 {#
  # Reinstalls packages using specified package manager.
  #}
 {%- macro pkg_reinstall(pkg_manager, pkg_manager_flags, packages) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
 {{ pkg_manager }} {{ helper.package_manager_flags(pkg_manager, pkg_manager_flags) }} {{ helper.package_manager_reinstall(pkg_manager) }} {%- for package in packages %} {{ package }}{% endfor %} \
-    {% if pkg_manager == 'microdnf' %}
-            && {{ pkg_manager }} clean all \
-    {% endif %}
-            && {{ helper.package_manager_query(pkg_manager) }}{% for package in packages %} {{ package.split('=')[0] }}{% endfor %}
-    {% endif %}
-{%- endmacro -%}
-
-{#
- # Clears repository cache using the specified package manager.
- #}
-{%- macro repo_clear_cache(pkg_manager) -%}
-    {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
-rm -rf "/var/cache/yum" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
-    {% endif %}
+            && {{ helper.package_manager_clean_and_query(pkg_manager) }}{% for package in packages %} {{ package.split('=')[0] }}{% endfor %}
 {%- endmacro -%}
 
 
@@ -233,7 +186,10 @@ rm -rf "/var/cache/yum" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
     {% for repo in animage.packages.repositories %}
     {% if repo.rpm %}
 
-    RUN {{ repo_install(animage.packages.manager, animage.packages.manager_flags, repo.rpm) }}
+    {#
+     # Installs a repository RPM using specified package manager.
+     #}
+    RUN {{ animage.packages.manager }} {{ helper.package_manager_flags(animage.packages.manager, animage.packages.manager_flags) }} install -y {{ repo.rpm }}
     {% endif %}
     {% endfor %}
     {% endif %}
@@ -280,15 +236,20 @@ rm -rf "/var/cache/yum" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
     RUN rm -rf "/tmp/scripts" "/tmp/artifacts"
     {% endif -%}
 
-    {% if helper.packages_to_install(image) or helper.packages_to_remove(image) or
-            helper.packages_to_reinstall(image) and image.packages.manager in ['yum', 'dnf', 'microdnf'] %}
+    {#
+     # Clears repository cache.
+     #}
+    {% if helper.package_manager_cleanup(image) %}
     # Clear package manager metadata
-    RUN {{ repo_clear_cache(image.packages.manager) }}
+    RUN rm -rf "/var/cache/yum" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
     {% endif -%}
 
+    {#
+     # Removes specified package repository from the system.
+     #}
     {% if image.packages.repositories_injected %}
     # Remove custom repo files
-    RUN {{ repo_remove(image.packages.manager, image.packages.repositories_injected) }}
+    RUN rm{% for repo in image.packages.repositories.injected %} /etc/yum.repos.d/{{ repo.filename }}{% endfor %}
     {% endif -%}
 
     {% if 'user' in run and image.from != 'scratch' %}
diff --git a/support/run_formatter.py b/support/run_formatter.py
index de54ba7..e345009 100755
--- a/support/run_formatter.py
+++ b/support/run_formatter.py
@@ -30,6 +30,7 @@ def main(check: bool, verbose: bool) -> None:
     verbose_opt = []
     if check:
         options.append("--check")
+        options.append("--diff")
     if verbose:
         logger.setLevel(logging.DEBUG)
         verbose_opt.append("--verbose")


Test patch:
diff --git a/tests/test_dockerfile.py b/tests/test_dockerfile.py
index 72cd2ae..3aa81a2 100644
--- a/tests/test_dockerfile.py
+++ b/tests/test_dockerfile.py
@@ -798,6 +798,11 @@ def test_package_removal_and_install_and_reinstall(tmpdir):
             && rpm -q t"""
         in dockerfile
     )
+    assert (
+        """RUN rm -rf "/var/cache/yum" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
+    """
+        in dockerfile
+    )
 
 
 def test_package_removal_without_install(tmpdir):
diff --git a/tests/test_validate.py b/tests/test_validate.py
index ee24f1b..ee945c6 100644
--- a/tests/test_validate.py
+++ b/tests/test_validate.py
@@ -575,8 +575,7 @@ def test_microdnf_clean_all_cmd_present(tmpdir):
 
     required_matches = [
         "RUN microdnf --setopt=install_weak_deps=0 --setopt=tsflags=nodocs install -y package1 package2 \\",
-        "&& microdnf clean all \\",
-        "&& rpm -q package1 package2",
+        "&& microdnf clean all && rpm -q package1 package2",
     ]
 
     for match in required_matches: