[libvirt] [jenkins-ci PATCH v2 1/3] guests: add openvz repository on CentOS 7

[Andrea Bolognani]

On Fri, 2019-12-06 at 18:53 +0000, Daniel P. Berrangé wrote:
> +++ b/guests/lcitool
> +    def _get_openvz_repo(self):
> +        basedir = os.path.dirname(sys.argv[0])
> +        repofile = os.path.join(basedir, "playbooks", "update", "templates", "openvz.repo.j2")

This should be

  base = Util.get_base()
  repofile = os.path.join(base, ...)

> +    def _get_openvz_key(self):
> +        basedir = os.path.dirname(sys.argv[0])
> +        repofile = os.path.join(basedir, "playbooks", "update", "templates", "openvz.key")

Same here, except you probably want to call it keyfile instead of
repofile.

> @@ -723,6 +735,16 @@ class Application:
>              elif os_name == "CentOS" and os_version == "7":
> +                repo = self._get_openvz_repo()
> +                repocmd = "\\n\\\n".join(repo.split("\n"))
> +                key = self._get_openvz_key()
> +                keycmd = "\\n\\\n".join(key.split("\n"))
> +
> +                sys.stdout.write(
> +                    "RUN echo -e '%s' > /etc/yum.repos.d/openvz.repo && \\\n" % repocmd +
> +                    "    echo -e '%s' > /etc/pki/rpm-gpg/RPM-GPG-KEY-OpenVZ && \\\n" % keycmd +
> +                    "    rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-OpenVZ")

This is different from what's right above and below it for, as far
as I can tell, no good reason.

You can make it nicer and more consistent like

  repo = self._get_openvz_repo()
  key = self._get_openvz_key()
  
  varmap["repo"] = "\\n\\\n".join(repo.split("\n"))
  varmap["key"] = "\\n\\\n".join(key.split("\n"))
  
  sys.stdout.write(textwrap.dedent("""
      RUN echo -e '{repo}' > /etc/yum.repos.d/openvz.repo && \\
          echo -e '{key}' > /etc/pki/rpm-gpg/RPM-GPG-KEY-OpenVZ && \\
          rpm --import /etc/pki/rpm-gpg/RPM-GPG-KEY-OpenVZ
  """).format(**varmap))
  
  sys.stdout.write(textwrap.dedent("""
      RUN {package_manager} update -y && \\
          {package_manager} install -y epel-release && \\
          {package_manager} install -y {pkgs} && \\
          {package_manager} autoremove -y && \\
          {package_manager} clean all -y
  """).format(**varmap))

or even merge the two RUN statements to reduce the number of layers
that will end up in the resulting image.


Everything else looks good, so with the above changed

  Reviewed-by: Andrea Bolognani <abologna at redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization