Re: [libvirt] [jenkins-ci PATCH v2 7/9] lcitool: avoid using an env var to store package 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