Repository: cekit/cekit (language: python)

Issue:
Opt-out from package manager cleanup 
**Describe the solution you'd like**
Hi! Is it possible to opt-out of the package manager metadata clean up? I can manage this by myself in a custom module.

**Additional context**
I'm trying to produce [reproducible builds for my images](https://reproducible-builds.org/) and this package manager cleanup that Cekit does at the end:

```Dockerfile
    # Clear package manager metadata
    RUN microdnf clean all && [ ! -d /var/cache/yum ] || rm -rf /var/cache/yum
```

Adds the `rpm` and `microdnf` cache and history I cleared before. These files contain non-deterministic files with timestamps and random UUID, making a reproducible build impossible.

I'm also opened to suggestions and how to approach this.
Opt-out from package manager cleanup 
**Describe the solution you'd like**
Hi! Is it possible to opt-out of the package manager metadata clean up? I can manage this by myself in a custom module.

**Additional context**
I'm trying to produce [reproducible builds for my images](https://reproducible-builds.org/) and this package manager cleanup that Cekit does at the end:

```Dockerfile
    # Clear package manager metadata
    RUN microdnf clean all && [ ! -d /var/cache/yum ] || rm -rf /var/cache/yum
```

Adds the `rpm` and `microdnf` cache and history I cleared before. These files contain non-deterministic files with timestamps and random UUID, making a reproducible build impossible.

I'm also opened to suggestions and how to approach this.
rhpam tests too sensitive to template changes
`tests/test_integ_builder_osbs.py::test_osbs_builder_with_rhpam_1`: the test has an _entire rendered Dockerfile_ embedded in it and checks for it byte-for-byte identical! This is going to break for any change we make to the template. I think that test (and any more like it) need refactoring to limit their comparisons to the topic of the test.

_Originally posted by @jmtd in https://github.com/cekit/cekit/issues/888#issuecomment-1934430943_
            


Discussion / hints:
Related to this is https://github.com/cekit/cekit/issues/849 where they wanted package manager cleanup after each install in each layer. 

There is also the point that I suspect `/var/cache/yum` might be outdated now - it should be (or be as well) `/var/cache/dnf`. And the `if` test is somewhat redundant with `rm -rf` (Also see [ DNF - Remove /var/cache/dnf files using the dnf clean command ](https://www.freekb.net/Article?id=3096) which implies the `clean all` could be reduced to a single `rm`)

In terms of package manager configuration - perhaps it could be more flexible offering the current choice (i.e. cleanup at end), cleanup per module, or no cleanup ? 

I'd vote for no cleanup at all and let the module or image handle that. I was maintaining some old codebase that was running the clean up in a "clean up" module. :) 

I understand that the tool shouldn't be opinionated to this point.
I don't think no cleanup is an option - that would be a rather surprising change to many users and effectively a 'breaking' change. However, I understand some users want more flexibility and manual control hence looking at configuration instead. 

@goldmann - thoughts?
I'm with you, @rnc. I think cleaning up by default is a sane default. If it would be possible to not make it as part of a switch, then that would be nice. But I imagine this is all-or-nothing option everything cleaned up or nothing cleaned up and left to module developer to tackle this.
+1 to add the on/off option for clean up.
I think the package management abstraction in `template.jinja` needs a little clean-up. At least with `manager: microdnf`, there's a `clean all` stage added to the end of each `install` as well. Thinking about this from a few different perspectives

 * I think supporting reproducible builds is a very good idea
 * if using layer squashing, from a speed perspective, doing all those `clean`s except the last are just costing build time
 * on the other hand if we are NOT using layer squashing then having the `clean` in is a good idea
 * the cleans after the installs don't perform the additional `rm -rf /var/cache/yum`
 * @ricardozanini 's problem is that the `clean all` command *creates* data (in `/var/lib/dnf` and `/var/lib/rpm`) that is not otherwise cleaned up: simply adding those directories to the list that is removed at the very end would be the simplest way to address the immediate problem (if it's safe for subsequent DNF/RPM operation in downstream images)

So I would propose extending the remove-at-end to cover the additional directories as a fast fix, and then look at whether we vary whether we clean up (and where we clean up) under what circumstances: different defaults for whether squashing or not? etc
Related to this is https://github.com/cekit/cekit/issues/849 where they wanted package manager cleanup after each install in each layer. 

There is also the point that I suspect `/var/cache/yum` might be outdated now - it should be (or be as well) `/var/cache/dnf`. And the `if` test is somewhat redundant with `rm -rf` (Also see [ DNF - Remove /var/cache/dnf files using the dnf clean command ](https://www.freekb.net/Article?id=3096) which implies the `clean all` could be reduced to a single `rm`)

In terms of package manager configuration - perhaps it could be more flexible offering the current choice (i.e. cleanup at end), cleanup per module, or no cleanup ? 

I'd vote for no cleanup at all and let the module or image handle that. I was maintaining some old codebase that was running the clean up in a "clean up" module. :) 

I understand that the tool shouldn't be opinionated to this point.
I don't think no cleanup is an option - that would be a rather surprising change to many users and effectively a 'breaking' change. However, I understand some users want more flexibility and manual control hence looking at configuration instead. 

@goldmann - thoughts?
I'm with you, @rnc. I think cleaning up by default is a sane default. If it would be possible to not make it as part of a switch, then that would be nice. But I imagine this is all-or-nothing option everything cleaned up or nothing cleaned up and left to module developer to tackle this.
+1 to add the on/off option for clean up.
I think the package management abstraction in `template.jinja` needs a little clean-up. At least with `manager: microdnf`, there's a `clean all` stage added to the end of each `install` as well. Thinking about this from a few different perspectives

 * I think supporting reproducible builds is a very good idea
 * if using layer squashing, from a speed perspective, doing all those `clean`s except the last are just costing build time
 * on the other hand if we are NOT using layer squashing then having the `clean` in is a good idea
 * the cleans after the installs don't perform the additional `rm -rf /var/cache/yum`
 * @ricardozanini 's problem is that the `clean all` command *creates* data (in `/var/lib/dnf` and `/var/lib/rpm`) that is not otherwise cleaned up: simply adding those directories to the list that is removed at the very end would be the simplest way to address the immediate problem (if it's safe for subsequent DNF/RPM operation in downstream images)

So I would propose extending the remove-at-end to cover the additional directories as a fast fix, and then look at whether we vary whether we clean up (and where we clean up) under what circumstances: different defaults for whether squashing or not? etc


Fix patch:
diff --git a/cekit/templates/template.jinja b/cekit/templates/template.jinja
index d11ab12e..1f7687da 100644
--- a/cekit/templates/template.jinja
+++ b/cekit/templates/template.jinja
@@ -74,7 +74,7 @@ rm{% for repo in repositories %} /etc/yum.repos.d/{{ repo.filename }}{% endfor %
  #}
 {%- macro repo_clear_cache(pkg_manager) -%}
     {% if pkg_manager in helper.SUPPORTED_PACKAGE_MANAGERS %}
-{{ pkg_manager }} clean all && [ ! -d /var/cache/yum ] || rm -rf /var/cache/yum
+rm -rf "/var/cache/yum" "/var/lib/rpm" "/var/lib/dnf" "/var/cache/apt" "/var/cache/dnf"
     {% endif %}
 {%- endmacro -%}
 
@@ -266,8 +266,7 @@ rm{% for repo in repositories %} /etc/yum.repos.d/{{ repo.filename }}{% endfor %
     {% if image.from != 'scratch' %}
     # Switch to 'root' user and remove artifacts and modules
     USER root
-    RUN [ ! -d /tmp/scripts ] || rm -rf /tmp/scripts
-    RUN [ ! -d /tmp/artifacts ] || rm -rf /tmp/artifacts
+    RUN rm -rf "/tmp/scripts" "/tmp/artifacts"
     {% endif -%}
 
     {% if helper.packages_to_install(image) or helper.packages_to_remove(image) or


Test patch:
diff --git a/tests/test_dockerfile.py b/tests/test_dockerfile.py
index 3a8912c5..47f2e2f1 100644
--- a/tests/test_dockerfile.py
+++ b/tests/test_dockerfile.py
@@ -449,7 +449,6 @@ def test_dockerfile_custom_package_manager_with_overrides(tmpdir):
         "RUN microdnf --setopt=install_weak_deps=0 --setopt=tsflags=nodocs install -y a b",
     )
     regex_dockerfile(target, "rpm -q a")
-    regex_dockerfile(target, "RUN microdnf clean all")
 
 
 # https://github.com/cekit/cekit/issues/462
@@ -480,7 +479,6 @@ def test_dockerfile_custom_package_manager_with_overrides_overriden_again(tmpdir
     regex_dockerfile(target, "RUN dnf --setopt=tsflags=nodocs install -y foo-repo.rpm")
     regex_dockerfile(target, "RUN dnf --setopt=tsflags=nodocs install -y a b")
     regex_dockerfile(target, "rpm -q a")
-    regex_dockerfile(target, "RUN dnf clean all")
 
 
 # https://github.com/cekit/cekit/issues/400
@@ -826,6 +824,29 @@ def test_args_buildah(tmpdir):
     )
 
 
+# https://github.com/cekit/cekit/issues/886
+def test_cleanup_rpm_dnf_default_pkg_manager(tmpdir):
+    target = str(tmpdir.mkdir("target"))
+
+    generate(
+        target,
+        ["-v", "build", "--dry-run", "--container-file", "Dockerfile", "podman"],
+        descriptor={
+            "packages": {
+                "manager": "microdnf",
+                "repositories": [{"name": "foo", "rpm": "foo-repo.rpm"}],
+                "install": ["a"],
+            },
+            "osbs": {"repository": {"name": "repo_name", "branch": "branch_name"}},
+        },
+    )
+    regex_dockerfile(target, "rm -rf.*/var/cache/yum")
+    regex_dockerfile(target, "rm -rf.*/var/lib/rpm")
+    regex_dockerfile(target, "rm -rf.*/var/lib/dnf")
+    regex_dockerfile(target, "rm -rf.*/var/cache/apt")
+    regex_dockerfile(target, "rm -rf.*/var/cache/dnf")
+
+
 def generate(image_dir, command, descriptor=None, exit_code=0):
     desc = basic_config.copy()
 
diff --git a/tests/test_integ_builder_osbs.py b/tests/test_integ_builder_osbs.py
index a770c3c1..8e85bdf3 100644
--- a/tests/test_integ_builder_osbs.py
+++ b/tests/test_integ_builder_osbs.py
@@ -2150,8 +2150,7 @@ def test_osbs_builder_with_rhpam_1(tmpdir, caplog):
 
     # Switch to 'root' user and remove artifacts and modules
     USER root
-    RUN [ ! -d /tmp/scripts ] || rm -rf /tmp/scripts
-    RUN [ ! -d /tmp/artifacts ] || rm -rf /tmp/artifacts
+    RUN rm -rf "/tmp/scripts" "/tmp/artifacts"
     # Define the user
     USER 1001
 ## /
@@ -2367,8 +2366,7 @@ def test_osbs_builder_with_rhpam_3(tmpdir, caplog):
 
     # Switch to 'root' user and remove artifacts and modules
     USER root
-    RUN [ ! -d /tmp/scripts ] || rm -rf /tmp/scripts
-    RUN [ ! -d /tmp/artifacts ] || rm -rf /tmp/artifacts
+    RUN rm -rf "/tmp/scripts" "/tmp/artifacts"
     # Define the user
     USER 1001
 ## /
diff --git a/tests/test_validate.py b/tests/test_validate.py
index 642744fc..f638d56b 100644
--- a/tests/test_validate.py
+++ b/tests/test_validate.py
@@ -1,6 +1,7 @@
 import logging
 import os
 import platform
+import re
 import shutil
 import sys
 import uuid
@@ -597,6 +598,13 @@ def check_dockerfile_text(image_dir, match, container_file="Dockerfile"):
     return False
 
 
+def regex_dockerfile(image_dir, exp_regex, container_file="Dockerfile"):
+    with open(os.path.join(image_dir, "target", "image", container_file), "r") as fd:
+        dockerfile_content = fd.read()
+        regex = re.compile(exp_regex, re.MULTILINE)
+        assert regex.search(dockerfile_content) is not None
+
+
 def check_dockerfile_uniq(image_dir, match, container_file="Dockerfile"):
     found = False
     with open(os.path.join(image_dir, "target", "image", container_file), "r") as fd:
@@ -1328,10 +1336,7 @@ def test_package_related_commands_packages_in_module(tmpdir, mocker):
 """
 
     assert check_dockerfile_text(image_dir, expected_packages_order_install)
-    assert check_dockerfile_text(
-        image_dir,
-        "RUN yum clean all && [ ! -d /var/cache/yum ] || rm -rf /var/cache/yum",
-    )
+    regex_dockerfile(image_dir, "rm -rf.*/var/cache/yum")
 
 
 def test_package_related_commands_packages_in_image(tmpdir, mocker):