[libvirt PATCH 2/4] ci: Rewrite list-images from Bash to Python

Erik Skultety posted 4 patches 4 years, 12 months ago
[libvirt PATCH 2/4] ci: Rewrite list-images from Bash to Python
Posted by Erik Skultety 4 years, 12 months ago
Convert the script to some human readable form. Speed-wise, both are
comparable.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 ci/Makefile       |  8 +-------
 ci/list-images.py | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 7 deletions(-)
 create mode 100644 ci/list-images.py

diff --git a/ci/Makefile b/ci/Makefile
index 1f71701047..92032e8ca0 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -217,13 +217,7 @@ ci-test@%:
 
 ci-list-images:
 	@echo
-	@echo "Available x86 container images:"
-	@echo
-	@sh list-images.sh "$(CI_IMAGE_PREFIX)" | grep -v cross
-	@echo
-	@echo "Available cross-compiler container images:"
-	@echo
-	@sh list-images.sh "$(CI_IMAGE_PREFIX)" | grep cross
+	@python3 list-images.py
 	@echo
 
 help:
diff --git a/ci/list-images.py b/ci/list-images.py
new file mode 100644
index 0000000000..6862ac347f
--- /dev/null
+++ b/ci/list-images.py
@@ -0,0 +1,29 @@
+#!/usr/bin/env python3
+
+import json
+import urllib.request as urllib
+
+PROJECT_ID = 192693
+DOMAIN = "https://gitlab.com"
+API = "api/v4"
+
+uri = f"{DOMAIN}/{API}/projects/{PROJECT_ID}/registry/repositories"
+r = urllib.urlopen(uri + "?per_page=100")
+
+# read the HTTP response and load the JSON part of it
+data = json.loads(r.read().decode())
+
+# skip the "ci-" prefix each of our container images' name has
+names = [i["name"][3:] for i in data]
+names.sort()
+
+names_native = [name for name in names if "cross" not in name]
+names_cross = [name for name in names if "cross" in name]
+
+print("Available x86 container images:\n")
+print("\t" + "\n\t".join(names_native))
+
+if names_cross:
+    print()
+    print("Available cross-compiler container images:\n")
+    print("\t" + "\n\t".join(names_cross))
-- 
2.29.2

Re: [libvirt PATCH 2/4] ci: Rewrite list-images from Bash to Python
Posted by Andrea Bolognani 4 years, 11 months ago
On Wed, 2021-02-10 at 18:24 +0100, Erik Skultety wrote:
> +# skip the "ci-" prefix each of our container images' name has
> +names = [i["name"][3:] for i in data]

I would prefer something like

  PREFIX = "ci-"
  names = [i["name"][len(PREFIX):] for i in data]

here, rather than hardcoding the length of the string.

> +names_native = [name for name in names if "cross" not in name]
> +names_cross = [name for name in names if "cross" in name]
> +
> +print("Available x86 container images:\n")
> +print("\t" + "\n\t".join(names_native))

Please use two or four spaces instead of tabs.

More high-level feedback about the patch:

  * you should drop the sh implementation at the same time as you're
    adding the python one;

  * adding this standalone script in one patch only to refactor most
    of its code away to a separate module in the next one leads to
    unnecessary churn and more work for the reviewer: please just add
    the script in its intended form in the first place.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH 2/4] ci: Rewrite list-images from Bash to Python
Posted by Erik Skultety 4 years, 11 months ago
On Tue, Feb 16, 2021 at 02:40:32PM +0100, Andrea Bolognani wrote:
> On Wed, 2021-02-10 at 18:24 +0100, Erik Skultety wrote:
> > +# skip the "ci-" prefix each of our container images' name has
> > +names = [i["name"][3:] for i in data]
> 
> I would prefer something like
> 
>   PREFIX = "ci-"
>   names = [i["name"][len(PREFIX):] for i in data]
> 
> here, rather than hardcoding the length of the string.
> 
> > +names_native = [name for name in names if "cross" not in name]
> > +names_cross = [name for name in names if "cross" in name]
> > +
> > +print("Available x86 container images:\n")
> > +print("\t" + "\n\t".join(names_native))
> 
> Please use two or four spaces instead of tabs.

Sure, will do.

> 
> More high-level feedback about the patch:
> 
>   * you should drop the sh implementation at the same time as you're
>     adding the python one;
> 
>   * adding this standalone script in one patch only to refactor most
>     of its code away to a separate module in the next one leads to
>     unnecessary churn and more work for the reviewer: please just add
>     the script in its intended form in the first place.

As you wish, I approached the way I'd appreciate it - introduce a drop 1:1
replacement and then change individual bits later...oh well, all of us have
different taste.

Thanks,
Erik