Every statement in a dockerfile results in a new layer in the
image. There is no need for an env var to store the package list
when it can be included inline. This avoids the env variable being
later exposed to the container at runtime.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 guests/lcitool | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index eb111b8..cd757eb 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -530,43 +530,46 @@ class Application:
                 if os_full in mappings[package]:
                     temp[package] = mappings[package][os_full]
 
-        flattened = []
+        pkgs = []
         for item in temp:
-            if temp[item] is not None and temp[item] not in flattened:
-                flattened += [temp[item]]
+            pkgname = temp[item]
+            if pkgname is None:
+                continue
+            if pkgname not in pkgs:
+                pkgs.append(pkgname)
 
         print("FROM {}".format(facts["docker_base"]))
 
-        sys.stdout.write("ENV PACKAGES ")
-        sys.stdout.write(" \\\n             ".join(sorted(flattened)))
-
+        varmap = {}
+        varmap["pkgs"] = "".join([" \\\n            " + pkgname
+                                  for pkgname in sorted(pkgs)])
         if package_format == "deb":
             sys.stdout.write(textwrap.dedent("""
                 RUN DEBIAN_FRONTEND=noninteractive && \\
                     ( \\
                         apt-get update && \\
                         apt-get dist-upgrade -y && \\
-                        apt-get install --no-install-recommends -y ${PACKAGES} && \\
+                        apt-get install --no-install-recommends -y %(pkgs)s && \\
                         apt-get autoremove -y && \\
                         apt-get autoclean -y \\
                     )
-            """))
+            """) % varmap )
         elif package_format == "rpm":
             if os_name == "Fedora" and os_version == "Rawhide":
                 sys.stdout.write(textwrap.dedent("""
                     RUN yum update -y --nogpgcheck fedora-gpg-keys && \\
                         yum update -y && \\
-                        yum install -y ${PACKAGES} && \\
+                        yum install -y %(pkgs)s && \\
                         yum autoremove -y && \\
                         yum clean all -y
-                """))
+                """) % varmap )
             else:
                 sys.stdout.write(textwrap.dedent("""
                     RUN yum update -y && \\
-                        yum install -y ${PACKAGES} && \\
+                        yum install -y %(pkgs)s && \\
                         yum autoremove -y && \\
                         yum clean all -y
-                """))
+                """) % varmap )
 
     def run(self):
         cmdline = self._parser.parse_args()
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote:
[...]
> -        flattened = []
> +        pkgs = []
>          for item in temp:
> -            if temp[item] is not None and temp[item] not in flattened:
> -                flattened += [temp[item]]
> +            pkgname = temp[item]
> +            if pkgname is None:
> +                continue
> +            if pkgname not in pkgs:
> +                pkgs.append(pkgname)

Nothing against refactoring the code this way, but such a change
belongs in its own patch.

[...]
> -        sys.stdout.write("ENV PACKAGES ")
> -        sys.stdout.write(" \\\n             ".join(sorted(flattened)))
> -
> +        varmap = {}
> +        varmap["pkgs"] = "".join([" \\\n            " + pkgname
> +                                  for pkgname in sorted(pkgs)])

Unlike the original, this pointlessly loops through sorted(pkgs)
one additional time and adds a phantom element to the beginning of
the list. So, once you put everything together...

>          if package_format == "deb":
>              sys.stdout.write(textwrap.dedent("""
>                  RUN DEBIAN_FRONTEND=noninteractive && \\
>                      ( \\
>                          apt-get update && \\
>                          apt-get dist-upgrade -y && \\
> -                        apt-get install --no-install-recommends -y ${PACKAGES} && \\
> +                        apt-get install --no-install-recommends -y %(pkgs)s && \\
>                          apt-get autoremove -y && \\
>                          apt-get autoclean -y \\
>                      )

... the result looks like

  apt-get install --no-install-recommends -y  \
      augeas-tools \
      autoconf \
      automake \
      ...

Notice the double space between '-y' and '\' as well as the weird
indentation. A better solution would be to do

  varmap["pkgs"] = " \\\n                ".join(sorted(pkgs))

followed by

  sys.stdout.write(textwrap.dedent("""
       ...
       apt-get install --no-install-recommends -y \\
               %(pkgs)s && \\
       apt-get autoremove -y && \\
       ...

which is clearer and results in the more pleasant

  apt-get install --no-install-recommends -y \
          augeas-tools \
          autoconf \
          automake \
          ...

> -            """))
> +            """) % varmap )

Again, I don't have a problem with using this syntax for string
formatting (and here it's actually much clearer than the one we
are currently using), but I don't want the script to become
inconsistent so we have to stick to a single style.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list