This makes the output more compact by grouping together all
images that are built on the same base OS.

Later on, when we change the actions that operate on
container images to accept an lcitool-style --cross-arch
argument instead of expecting the name of the image to have
the cross-building architecture baked in, this will allow
users to quickly copy-and-paste the necessary information.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 ci/helper | 23 +++++------------------
 1 file changed, 5 insertions(+), 18 deletions(-)

diff --git a/ci/helper b/ci/helper
index c1e1a53d51..6543a47131 100755
--- a/ci/helper
+++ b/ci/helper
@@ -311,25 +311,12 @@ class Application:
                                              self._args.gitlab_uri)
         images = util.get_registry_images(registry_uri)
 
-        native = []
-        cross = []
-        for image_distro in images.keys():
-            if images[image_distro]:
-                for image_cross in images[image_distro]:
-                    cross.append(image_distro + "-cross-" + image_cross)
+        for distro in sorted(images.keys()):
+            if images[distro]:
+                cross = " ".join(sorted(images[distro]))
+                print(f"{distro} (cross: {cross})")
             else:
-                native.append(image_distro)
-        native.sort()
-        cross.sort()
-
-        spacing = 4 * " "
-        print("Available x86 container images:\n")
-        print(spacing + ("\n" + spacing).join(native))
-
-        if cross:
-            print()
-            print("Available cross-compiler container images:\n")
-            print(spacing + ("\n" + spacing).join(cross))
+                print(distro)
 
     def _action_refresh(self):
         self._refresh_containers()
-- 
2.26.3

On Fri, Apr 23, 2021 at 05:03:04PM +0200, Andrea Bolognani wrote:
> This makes the output more compact by grouping together all
> images that are built on the same base OS.


Yes, it definitely does make the output more compact, but I'm still not
completely sold on the idea that this is an actual improvement...

> 
> Later on, when we change the actions that operate on
> container images to accept an lcitool-style --cross-arch
> argument instead of expecting the name of the image to have
> the cross-building architecture baked in, this will allow
> users to quickly copy-and-paste the necessary information.

...you can quickly copy-and-paste the image name even now and I would argue
that it's even quicker than after this series:

./helper list-images
./helper <action> <copy-paste image name>  vs

./helper list-images
./helper <action> <copy-paste image name> --cross-arch <copy-paste cross arch>

Right now you wouldn't even need the verification code introduced in patch 12
since you have to paste the image verbatim which IMO for this utility helper
that only serves a very specific use case of a single repo is "good enough".
I guess the idea is to eventually integrate this helper somehow to lcitool, so
I'm wondering what is it we're trying to solve here. As for the output itself,
if we want to change it, then I'd be probably more inclined towards something
like virt-builder --list. There's massive information redundancy in there -
no need to argue - but because the output is formatted like a simple table,
tools like grep,sort,uniq and cut make the customization of the output for the
average linux user an absolute no-brainer and they always get information they
were looking for easily with almost no added effort.

Erik

On Mon, Apr 26, 2021 at 11:59:19AM +0200, Erik Skultety wrote:
> On Fri, Apr 23, 2021 at 05:03:04PM +0200, Andrea Bolognani wrote:
> > Later on, when we change the actions that operate on
> > container images to accept an lcitool-style --cross-arch
> > argument instead of expecting the name of the image to have
> > the cross-building architecture baked in, this will allow
> > users to quickly copy-and-paste the necessary information.
>
> ...you can quickly copy-and-paste the image name even now and I would argue
> that it's even quicker than after this series:
>
> ./helper list-images
> ./helper <action> <copy-paste image name>  vs
>
> ./helper list-images
> ./helper <action> <copy-paste image name> --cross-arch <copy-paste cross arch>
>
> Right now you wouldn't even need the verification code introduced in patch 12
> since you have to paste the image verbatim which IMO for this utility helper
> that only serves a very specific use case of a single repo is "good enough".

That assumes the target name only comes from copying and pasting from
the output of the list-images action... Right now, if you pass some
invalid / obsolete value you get

  $ ./ci/helper build ubuntu-1604
  make: Entering directory '/home/abologna/src/upstream/libvirt/ci'
  make -C /home/abologna/src/upstream/libvirt/ci
ci-run-command@ubuntu-1604 CI_COMMAND="/home/abologna/build"
  make[1]: Entering directory '/home/abologna/src/upstream/libvirt/ci'
  Checking if podman is available...yes
  Cloning /home/abologna/src/upstream/libvirt to
/home/abologna/src/upstream/libvirt/ci/scratch/src
  Cloning /home/abologna/src/upstream/libvirt/src/keycodemapdb to
/home/abologna/src/upstream/libvirt/ci/scratch/src/src/keycodemapdb
  podman run \
  	--rm --interactive --tty --user "1000":"1000" --workdir
"/home/abologna" --env CI_CONT_SRCDIR="/home/abologna/libvirt" --env
CI_MESON_ARGS="" --env CI_NINJA_ARGS="" --uidmap 0:1:1000 --uidmap
1000:0:1 --uidmap 1001:1001:64536 --gidmap 0:1:1000 --gidmap 1000:0:1
--gidmap 1001:1001:64536  --volume
/home/abologna/src/upstream/libvirt/ci/scratch/group:/etc/group:ro,z
--volume /home/abologna/src/upstream/libvirt/ci/scratch/passwd:/etc/passwd:ro,z
 --volume /home/abologna/src/upstream/libvirt/ci/scratch/home:/home/abologna:z
 --volume /home/abologna/src/upstream/libvirt/ci/scratch/build:/home/abologna/build:z
 --volume /home/abologna/src/upstream/libvirt/ci/scratch/src:/home/abologna/libvirt:z
--ulimit nofile=1024:1024 --cap-add=SYS_PTRACE  \
  	registry.gitlab.com/libvirt/libvirt/ci-ubuntu-1604:latest \
  	/home/abologna/build
  Trying to pull registry.gitlab.com/libvirt/libvirt/ci-ubuntu-1604:latest...
    manifest unknown: manifest unknown
  Error: Error initializing source
docker://registry.gitlab.com/libvirt/libvirt/ci-ubuntu-1604:latest:
Error reading manifest latest in
registry.gitlab.com/libvirt/libvirt/ci-ubuntu-1604: manifest unknown:
manifest unknown
  make[1]: *** [Makefile:199: ci-run-command@ubuntu-1604] Error 125
  make[1]: Leaving directory '/home/abologna/src/upstream/libvirt/ci'
  make: *** [Makefile:209: ci-build@ubuntu-1604] Error 2
  make: Leaving directory '/home/abologna/src/upstream/libvirt/ci'
  error: 'make' failed

With my patches, the output is a much more reasonable

  $ ./ci/helper build ubuntu-1604
  Unknown target 'ubuntu-1604'

I agree that having to input the target OS and the target
architecture separately is slightly more copy-and-paste work, but
really those are two semantically separate bits of information and
it's just wrong for the user to provide a single argument that
encodes both.

It was an acceptable trade-off when we were using make, but now that
we have an actual command-line parser at our disposal we should take
advantage of it.

> I guess the idea is to eventually integrate this helper somehow to lcitool, so
> I'm wondering what is it we're trying to solve here. As for the output itself,
> if we want to change it, then I'd be probably more inclined towards something
> like virt-builder --list. There's massive information redundancy in there -
> no need to argue - but because the output is formatted like a simple table,
> tools like grep,sort,uniq and cut make the customization of the output for the
> average linux user an absolute no-brainer and they always get information they
> were looking for easily with almost no added effort.

Changing the output to be similar to that of 'virt-builder --list'
sounds good to me.