[libvirt] [jenkins-ci PATCH 0/2] lcitool: Refactor Dockerfile generation
Andrea Bolognani (2): lcitool: Perform system update after enabling repos lcitool: Refactor Dockerfile generation guests/lcitool | 66 +++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) -- 2.23.0
Both orders work and lead to the same results, but performing the steps in this specific order will make further refactoring easier. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- guests/lcitool | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index d4dc1c8..4c2a04e 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -751,18 +751,18 @@ class Application: elif os_name == "CentOS": if os_version == "7": sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} update -y && \\ - {package_manager} install -y epel-release && \\ + RUN {package_manager} install -y epel-release && \\ + {package_manager} update -y && \\ {package_manager} install -y {pkgs} && \\ {package_manager} autoremove -y && \\ {package_manager} clean all -y """).format(**varmap)) else: sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} update -y && \\ - {package_manager} install 'dnf-command(config-manager)' -y && \\ + RUN {package_manager} install 'dnf-command(config-manager)' -y && \\ {package_manager} config-manager --set-enabled PowerTools -y && \\ {package_manager} install -y epel-release && \\ + {package_manager} update -y && \\ {package_manager} install -y {pkgs} && \\ {package_manager} autoremove -y && \\ {package_manager} clean all -y -- 2.23.0
The current code is quite a mess, with the same commands being repeated over and over again with very minor variations based on necessities that are not spelled out at all. Refactor it and solve both issues in the process; the output is entirely unchanged. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- guests/lcitool | 66 +++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 4c2a04e..9958508 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -740,40 +740,40 @@ class Application: {package_manager} autoclean -y """).format(**varmap)) elif package_format == "rpm": + commands = [] + + # Rawhide needs this because the keys used to sign packages are + # cycled from time to time if os_name == "Fedora" and os_version == "Rawhide": - sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} update -y --nogpgcheck fedora-gpg-keys && \\ - {package_manager} update -y && \\ - {package_manager} install -y {pkgs} && \\ - {package_manager} autoremove -y && \\ - {package_manager} clean all -y - """).format(**varmap)) - elif os_name == "CentOS": - if os_version == "7": - sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} install -y epel-release && \\ - {package_manager} update -y && \\ - {package_manager} install -y {pkgs} && \\ - {package_manager} autoremove -y && \\ - {package_manager} clean all -y - """).format(**varmap)) - else: - sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} install 'dnf-command(config-manager)' -y && \\ - {package_manager} config-manager --set-enabled PowerTools -y && \\ - {package_manager} install -y epel-release && \\ - {package_manager} update -y && \\ - {package_manager} install -y {pkgs} && \\ - {package_manager} autoremove -y && \\ - {package_manager} clean all -y - """).format(**varmap)) - else: - sys.stdout.write(textwrap.dedent(""" - RUN {package_manager} update -y && \\ - {package_manager} install -y {pkgs} && \\ - {package_manager} autoremove -y && \\ - {package_manager} clean all -y - """).format(**varmap)) + commands.extend([ + "{package_manager} update -y --nogpgcheck fedora-gpg-keys" + ]) + + if os_name == "CentOS": + # Starting with CentOS 8, most -devel packages are shipped in + # the PowerTools repository, which is not enabled by default + if os_version != "7": + commands.extend([ + "{package_manager} install 'dnf-command(config-manager)' -y", + "{package_manager} config-manager --set-enabled PowerTools -y", + ]) + + # Some of the packages we need are not part of CentOS proper + # and are only available through EPEL + commands.extend([ + "{package_manager} install -y epel-release", + ]) + + commands.extend([ + "{package_manager} update -y", + "{package_manager} install -y {pkgs}", + "{package_manager} autoremove -y", + "{package_manager} clean all -y", + ]) + + script = "\nRUN " + (" && \\\n ".join(commands)) + "\n" + + sys.stdout.write(script.format(**varmap)) if pip_pkgs: sys.stdout.write(textwrap.dedent(""" -- 2.23.0
[snip]
+ + commands.extend([ + "{package_manager} update -y", + "{package_manager} install -y {pkgs}", + "{package_manager} autoremove -y",
This is going to be fun when we enable OpenSUSE support for container generation. `zypper autoremove -y` is not a valid command and we'll have to break this part. With this in mind, I'm not sure whether it'd be worth to have a proper mapping of the distros and the commands they support, having Fedora / CentOS as the base. What do you think? Mind, I'm not pushing for the mapping, just pointing it out. :-). I'm fine with an `if os_name != "OpenSUSE": ...` [snip] Best Regards, -- Fabiano Fidêncio
On Thu, 2019-12-12 at 14:49 +0100, Fabiano Fidêncio wrote:
+ commands.extend([ + "{package_manager} update -y", + "{package_manager} install -y {pkgs}", + "{package_manager} autoremove -y",
This is going to be fun when we enable OpenSUSE support for container generation. `zypper autoremove -y` is not a valid command and we'll have to break this part.
I already have patches for openSUSE locally - I'm actually testing them as we speak :)
With this in mind, I'm not sure whether it'd be worth to have a proper mapping of the distros and the commands they support, having Fedora / CentOS as the base.
What do you think?
Mind, I'm not pushing for the mapping, just pointing it out. :-). I'm fine with an `if os_name != "OpenSUSE": ...`
Yeah, even with all this it's still kinda yucky, but much better than what we have now. Adding another level of mapping doesn't seem worth it to me, but feel free to experiment with it and send patches if you end up with something that improves on the status quo. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Dec 12, 2019 at 3:05 PM Andrea Bolognani <abologna@redhat.com> wrote:
On Thu, 2019-12-12 at 14:49 +0100, Fabiano Fidêncio wrote:
+ commands.extend([ + "{package_manager} update -y", + "{package_manager} install -y {pkgs}", + "{package_manager} autoremove -y",
This is going to be fun when we enable OpenSUSE support for container generation. `zypper autoremove -y` is not a valid command and we'll have to break this part.
I already have patches for openSUSE locally - I'm actually testing them as we speak :)
Oh, me too. :-)
With this in mind, I'm not sure whether it'd be worth to have a proper mapping of the distros and the commands they support, having Fedora / CentOS as the base.
What do you think?
Mind, I'm not pushing for the mapping, just pointing it out. :-). I'm fine with an `if os_name != "OpenSUSE": ...`
Yeah, even with all this it's still kinda yucky, but much better than what we have now. Adding another level of mapping doesn't seem worth it to me, but feel free to experiment with it and send patches if you end up with something that improves on the status quo.
I'm fine with the status quo.
-- Andrea Bolognani / Red Hat / Virtualization
On Thu, Dec 12, 2019 at 2:23 PM Andrea Bolognani <abologna@redhat.com> wrote:
Andrea Bolognani (2): lcitool: Perform system update after enabling repos lcitool: Refactor Dockerfile generation
guests/lcitool | 66 +++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 33 deletions(-)
-- 2.23.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
participants (2)
-
Andrea Bolognani -
Fabiano Fidêncio