[libvirt PATCH v2 0/6] ci: helper: Rewrite image listing in Python and add stale Docker image checker
Technically v2 of https://listman.redhat.com/archives/libvir-list/2021-February/msg00641.html, but the changes are not minor anymore so I had to change the subject. since v1: - integrated the standalone script to the recently added 'helper' script - ditched the hardcode libvirt project ID and instead we work with namespacing instead (which is converted to the right URL under the hood) - util module moved to the ci/ directory - all util functions now use proper Python type hinting - ID is now printed along with the stale image name - address formatting issues raised v1: https://listman.redhat.com/archives/libvir-list/2021-February/msg00641.ht= ml EXAMPLE OUTPUT OF THE REGISTRY CHECKER: The following images can be purged from the registry: ci-debian-9: 1154661 ci-debian-9-cross-aarch64: 1154667 ci-debian-9-cross-mipsel: 1154669 ci-debian-9-cross-armv7l: 1154671 ci-debian-9-cross-armv6l: 1154676 ci-debian-9-cross-mips: 1154678 ci-debian-9-cross-ppc64le: 1154682 ci-debian-9-cross-s390x: 1154683 ci-debian-9-cross-mips64el: 1154686 ci-fedora-31: 1154687 ci-opensuse-151: 1154724 You can remove the above images over the API with the following code snippet: $ for image_id in 1154661 1154667 1154669 1154671 1154676 1154678 1154682= \ 1154683 1154686 1154687 1154724; do \ curl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \ https://gitlab.com/api/v4/projects/libvirt%2Flibvirt/registry/repos= itories/$image_id \ done Erik Skultety (6): ci: Makefile: Specify a help target to replace ci-help ci: helper: Use the ArgumentDefaultsHelpFormatter help formatter ci: helper: Introduce --quiet ci: Introduce a util module ci: helper: Rewrite image listing to Python ci: util: Add a registry checker for stale images ci/Makefile | 16 ++----- ci/helper | 108 ++++++++++++++++++++++++++++++++++++++++++++-- ci/list-images.sh | 14 ------ ci/util.py | 52 ++++++++++++++++++++++ 4 files changed, 159 insertions(+), 31 deletions(-) delete mode 100644 ci/list-images.sh create mode 100644 ci/util.py --=20 2.29.2
It's quite pointless to have a 'ci-help' target in the Makefile when one needs to actually open the Makefile to go through the list of targets to know what functionality the Makefile actually provides. It's much more intuitive to run "make help" in that case. Therefore, add a 'help' target and replace the old 'ci-help' target with it. Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- ci/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/Makefile b/ci/Makefile index 72f5bda942..f83ecac1e5 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -222,7 +222,7 @@ ci-list-images: @sh list-images.sh "$(CI_IMAGE_PREFIX)" | grep cross @echo -ci-help: +help: @echo @echo @echo @@ -242,7 +242,7 @@ ci-help: @echo " ci-test@\$$IMAGE - run a 'ninja test'" @echo " ci-shell@\$$IMAGE - run an interactive shell" @echo " ci-list-images - list available images" - @echo " ci-help - show this help message" + @echo " help - show this help message" @echo @echo "Available make variables:" @echo -- 2.29.2
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
It's quite pointless to have a 'ci-help' target in the Makefile when one needs to actually open the Makefile to go through the list of targets to know what functionality the Makefile actually provides. It's much more intuitive to run "make help" in that case. Therefore, add a 'help' target and replace the old 'ci-help' target with it.
Signed-off-by: Erik Skultety <eskultet@redhat.com> Reviewed-by: Andrea Bolognani <abologna@redhat.com> --- ci/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Since we now expect users to call the ci/helper script rather than using ci/Makefile directly, this is unnecessary. Please drop it from the series. -- Andrea Bolognani / Red Hat / Virtualization
This help formatter class reports the defaults we use for options taking an argument. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ci/helper b/ci/helper index 73a3f729da..8f34f4b59d 100755 --- a/ci/helper +++ b/ci/helper @@ -79,6 +79,7 @@ class Parser: "build", help="run a build in a container", parents=[containerparser, mesonparser], + formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) buildparser.set_defaults(func=Application.action_build) @@ -87,6 +88,7 @@ class Parser: "test", help="run a build in a container (including tests)", parents=[containerparser, mesonparser], + formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) testparser.set_defaults(func=Application.action_test) @@ -95,6 +97,7 @@ class Parser: "shell", help="start a shell in a container", parents=[containerparser], + formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) shellparser.set_defaults(func=Application.action_shell) @@ -102,6 +105,7 @@ class Parser: listimagesparser = subparsers.add_parser( "list-images", help="list known container images", + formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) listimagesparser.set_defaults(func=Application.action_list_images) @@ -110,6 +114,7 @@ class Parser: "refresh", help="refresh data generated with lcitool", parents=[lcitoolparser], + formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) refreshparser.set_defaults(func=Application.action_refresh) -- 2.29.2
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
This help formatter class reports the defaults we use for options taking an argument.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 5 +++++ 1 file changed, 5 insertions(+)
It's a shame that we have to repeat this for every single action instead of being able to set it for the top-level parser and have subparser inherit that. Oh well. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 11:52:22AM +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
This help formatter class reports the defaults we use for options taking an argument.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 5 +++++ 1 file changed, 5 insertions(+)
It's a shame that we have to repeat this for every single action instead of being able to set it for the top-level parser and have subparser inherit that. Oh well.
Yep, I was sad as well when I found out.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Thanks, Erik
Offer an option to silence all output to stdout, e.g. when generating dockerfiles/varsfiles. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ci/helper b/ci/helper index 8f34f4b59d..abac686d22 100755 --- a/ci/helper +++ b/ci/helper @@ -68,6 +68,12 @@ class Parser: # Main parser self.parser = argparse.ArgumentParser() + self.parser.add_argument( + "--quiet", + action="store_true", + default=False, + help="silence all output" + ) subparsers = self.parser.add_subparsers( dest="action", metavar="ACTION", @@ -173,7 +179,8 @@ class Application: outfile = f"ci-{host}-cross-{cross}.Dockerfile" outpath = outdir.joinpath(outfile) - print(outpath) + if not self.args.quiet: + print(outpath) output = self.lcitool_run(args) with open(outpath, "w") as f: @@ -185,7 +192,8 @@ class Application: outfile = f"{host}.vars" outpath = outdir.joinpath(outfile) - print(outpath) + if not self.args.quiet: + print(outpath) output = self.lcitool_run(args) with open(outpath, "w") as f: -- 2.29.2
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+++ b/ci/helper @@ -68,6 +68,12 @@ class Parser:
# Main parser self.parser = argparse.ArgumentParser() + self.parser.add_argument( + "--quiet", + action="store_true", + default=False, + help="silence all output" + )
This is not an entirely accurate description, because something like $ ci/helper --quiet build debian-10 will still produce plenty of output... We can fix that afterwards though. Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 11:54:40AM +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+++ b/ci/helper @@ -68,6 +68,12 @@ class Parser:
# Main parser self.parser = argparse.ArgumentParser() + self.parser.add_argument( + "--quiet", + action="store_true", + default=False, + help="silence all output" + )
This is not an entirely accurate description, because something like
$ ci/helper --quiet build debian-10
will still produce plenty of output... We can fix that afterwards though.
Hmm, the question then is whether it actually makes sense with any other command besides refresh? You don't want to suppress building output, because what's the point, you don't want to do it with 'shell' and 'list-images' either (obviously), leaving us essentially with 'refresh' only. So I'll change this accordingly. Erik
With the gradual rewrite of the Makefile to the 'helper' script will require helper functions that would better live in a separate util module. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/util.py | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 ci/util.py diff --git a/ci/util.py b/ci/util.py new file mode 100644 index 0000000000..8a2d6d8f47 --- /dev/null +++ b/ci/util.py @@ -0,0 +1,39 @@ +import json +import urllib.request as urllibrequest +import urllib.parse as urllibparse + +from typing import Dict, List + + +def get_registry_uri(namespace: str, + gitlab_uri="https://gitlab.com", + api_version=4) -> str: + """ + :param namespace: GitLab project namespace, e.g. "libvirt/libvirt" + :param gitlab_uri: GitLab base URI, can be a private deployment + :param api_version: GitLab REST API version number + :return: URI pointing to a namespaced project's image registry + """ + + # this converts something like "libvirt/libvirt" to "libvirt%2Flibvirt" + namespace_urlenc = urllibparse.quote_plus(namespace) + + apistr = str(api_version) + project_uri = f"{gitlab_uri}/api/v{apistr}/projects/{namespace_urlenc}" + + uri = project_uri + "/registry/repositories" + return uri + + +def get_registry_images(uri: str) -> Dict[str, str]: + """ + Returns all container images as currently available for the given GitLab + project. + + :return: list of container image names + """ + + r = urllibrequest.urlopen(uri + "?per_page=100") + + # read the HTTP response and load the JSON part of it + return json.loads(r.read().decode()) -- 2.29.2
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+import urllib.request as urllibrequest +import urllib.parse as urllibparse
The aliasing doesn't look like it serves a real purpose here, as it only saves a single character... I'd import, and use, urrlib.request and urllib.parse directly.
+ apistr = str(api_version) + project_uri = f"{gitlab_uri}/api/v{apistr}/projects/{namespace_urlenc}"
Can't you use api_version without explicitly casting/converting it to a string here? It seems to work. With these two nits addressed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2021-03-17 at 12:02 +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+ apistr = str(api_version) + project_uri = f"{gitlab_uri}/api/v{apistr}/projects/{namespace_urlenc}"
Can't you use api_version without explicitly casting/converting it to a string here? It seems to work.
Actually, considering that our code only really speaks v4 API, it would make perfect sense to drop the api_version argument altogether and hardcode "v4" in the returned URI. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 12:02:31PM +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+import urllib.request as urllibrequest +import urllib.parse as urllibparse
The aliasing doesn't look like it serves a real purpose here, as it only saves a single character... I'd import, and use, urrlib.request and urllib.parse directly.
Okay, I can do that, the point was not to import urllib as is, because that may not work reliably - for some reason Python refuses to import giant modules and will report an error that it could not find a symbol, so you have to import a submodule instead.
+ apistr = str(api_version) + project_uri = f"{gitlab_uri}/api/v{apistr}/projects/{namespace_urlenc}"
Can't you use api_version without explicitly casting/converting it to a string here? It seems to work.
I have to re-try, but at some point Python was complaining about it being an int.
With these two nits addressed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
-- Andrea Bolognani / Red Hat / Virtualization
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+++ b/ci/util.py +def get_registry_images(uri: str) -> Dict[str, str]:
On closer inspection, the return type is not correct: we return a list of JSON objects, not a mapping from strings to strings. I don't know how to express that in terms of Python types (List[Dict[str, Any]]?), and I'm concerned by the fact that changing the return type to something obviously wrong like "int" results in zero observable changes. How are type hints enforced? Do we need to turn that on somehow? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 05:06:33PM +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+++ b/ci/util.py +def get_registry_images(uri: str) -> Dict[str, str]:
On closer inspection, the return type is not correct: we return a list of JSON objects, not a mapping from strings to strings.
Good catch.
I don't know how to express that in terms of Python types (List[Dict[str, Any]]?), and I'm concerned by the fact that changing the return type to something obviously wrong like "int" results in zero observable changes.
I'd suggest List[Dict] to be enough in this case since. We can safely assume the returned JSON will always be a list of objects, the rest is on the caller.
How are type hints enforced? Do we need to turn that on somehow?
They're not. Python will always remain a dynamically typed language. The whole point of type hinting is for static analysis via 'mypy' and for various IDEs that can actually make use of those as well during completion which is nice. (I'm wondering whether the jedi vim plugin is capable of reading those too) Erik
On Wed, 2021-03-17 at 18:05 +0100, Erik Skultety wrote:
On Wed, Mar 17, 2021 at 05:06:33PM +0100, Andrea Bolognani wrote:
I don't know how to express that in terms of Python types (List[Dict[str, Any]]?), and I'm concerned by the fact that changing the return type to something obviously wrong like "int" results in zero observable changes.
I'd suggest List[Dict] to be enough in this case since. We can safely assume the returned JSON will always be a list of objects, the rest is on the caller.
I trust your expertise here :)
How are type hints enforced? Do we need to turn that on somehow?
They're not. Python will always remain a dynamically typed language. The whole point of type hinting is for static analysis via 'mypy' and for various IDEs that can actually make use of those as well during completion which is nice. (I'm wondering whether the jedi vim plugin is capable of reading those too)
I see. It would be nice to run mypy in 'check' mode (assuming that exists) at syntax-check time, similarly to what we already do with flake8. That can come later, though. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 06:11:32PM +0100, Andrea Bolognani wrote:
On Wed, 2021-03-17 at 18:05 +0100, Erik Skultety wrote:
On Wed, Mar 17, 2021 at 05:06:33PM +0100, Andrea Bolognani wrote:
I don't know how to express that in terms of Python types (List[Dict[str, Any]]?), and I'm concerned by the fact that changing the return type to something obviously wrong like "int" results in zero observable changes.
I'd suggest List[Dict] to be enough in this case since. We can safely assume the returned JSON will always be a list of objects, the rest is on the caller.
I trust your expertise here :)
How are type hints enforced? Do we need to turn that on somehow?
They're not. Python will always remain a dynamically typed language. The whole point of type hinting is for static analysis via 'mypy' and for various IDEs that can actually make use of those as well during completion which is nice. (I'm wondering whether the jedi vim plugin is capable of reading those too)
I see. It would be nice to run mypy in 'check' mode (assuming that exists) at syntax-check time, similarly to what we already do with flake8. That can come later, though.
One more thing, what is the Python 3.<minor> acceptance policy for scripts? I mean we expect the scripts to work with 3.X as we declare on libvirt.org, but f-strings for example are 3.6+ and 3.9 essentially makes the typing module deprecated since it integrated most of the hints to standard collections [1], IOW instead of writing "List[Dict[type, type]]" you can now use the standard containers for that as well: "list[dict[type, type]]" without importing anything, so basically the list/dict builtin can serve as both container constructors as well as static type hints. Erik [1] https://www.python.org/dev/peps/pep-0585/
On Wed, 2021-03-17 at 18:40 +0100, Erik Skultety wrote:
One more thing, what is the Python 3.<minor> acceptance policy for scripts? I mean we expect the scripts to work with 3.X as we declare on libvirt.org, but f-strings for example are 3.6+ and 3.9 essentially makes the typing module deprecated since it integrated most of the hints to standard collections [1], IOW instead of writing "List[Dict[type, type]]" you can now use the standard containers for that as well: "list[dict[type, type]]" without importing anything, so basically the list/dict builtin can serve as both container constructors as well as static type hints.
All target platforms, including CentOS 7 and Debian 10, have 3.6+ so it's okay to use f-strings. -- Andrea Bolognani / Red Hat / Virtualization
The corresponding Bash script is dropped. After this patch's rewrite, the Makefile's original image listing target remains intact only to notify the user to use the Python helper instead. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/Makefile | 12 +----------- ci/helper | 37 ++++++++++++++++++++++++++++++++++++- ci/list-images.sh | 14 -------------- 3 files changed, 37 insertions(+), 26 deletions(-) delete mode 100644 ci/list-images.sh diff --git a/ci/Makefile b/ci/Makefile index f83ecac1e5..7720dddfb9 100644 --- a/ci/Makefile +++ b/ci/Makefile @@ -211,16 +211,7 @@ ci-build@%: ci-test@%: $(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_NINJA_ARGS=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 - @echo +ci-list-images: help help: @echo @@ -241,7 +232,6 @@ help: @echo " ci-build@\$$IMAGE - run a default 'ninja' build" @echo " ci-test@\$$IMAGE - run a 'ninja test'" @echo " ci-shell@\$$IMAGE - run an interactive shell" - @echo " ci-list-images - list available images" @echo " help - show this help message" @echo @echo "Available make variables:" diff --git a/ci/helper b/ci/helper index abac686d22..1b7675c58a 100755 --- a/ci/helper +++ b/ci/helper @@ -11,6 +11,8 @@ import shutil import subprocess import sys +import util + class Parser: def __init__(self): @@ -66,6 +68,20 @@ class Parser: help="path to lcitool binary", ) + # Options that are common to actions communicating with a GitLab + # instance + gitlabparser = argparse.ArgumentParser(add_help=False) + gitlabparser.add_argument( + "--namespace", + default="libvirt/libvirt", + help="GitLab project namespace" + ) + gitlabparser.add_argument( + "--gitlab-uri", + default="https://gitlab.com", + help="base GitLab URI" + ) + # Main parser self.parser = argparse.ArgumentParser() self.parser.add_argument( @@ -111,6 +127,7 @@ class Parser: listimagesparser = subparsers.add_parser( "list-images", help="list known container images", + parents=[gitlabparser], formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) listimagesparser.set_defaults(func=Application.action_list_images) @@ -249,7 +266,25 @@ class Application: self.make_run(f"ci-shell@{self.args.target}") def action_list_images(self): - self.make_run(f"ci-list-images") + registry_uri = util.get_registry_uri(self.args.namespace, + self.args.gitlab_uri) + images = util.get_registry_images(registry_uri) + + # skip the "ci-" prefix each of our container images' name has + name_prefix = "ci-" + names = [i["name"][len(name_prefix):] for i in images] + names.sort() + + native = [name for name in names if "-cross-" not in name] + cross = [name for name in names if "-cross-" in name] + + print("Available x86 container images:\n") + print("\t" + "\n\t".join(native)) + + if cross: + print() + print("Available cross-compiler container images:\n") + print("\t" + "\n\t".join(cross)) def action_refresh(self): self.refresh_containers() diff --git a/ci/list-images.sh b/ci/list-images.sh deleted file mode 100644 index b85b132253..0000000000 --- a/ci/list-images.sh +++ /dev/null @@ -1,14 +0,0 @@ -#!/bin/sh - -prefix="${1##registry.gitlab.com/}" - -PROJECT_ID=192693 - -all_repos() { - curl -s "https://gitlab.com/api/v4/projects/$PROJECT_ID/registry/repositories?per_pag..." \ - | tr , '\n' | grep '"path":' | sed 's,"path":",,g;s,"$,,g' -} - -all_repos | grep "^$prefix" | sed "s,^$prefix,,g" | while read repo; do - echo " $repo" -done | sort -u -- 2.29.2
On Tue, 2021-03-16 at 18:32 +0100, Erik Skultety wrote:
+ci-list-images: help
I'd just drop the target altogether. The entire Makefile is hopefully going to disappear soon enough anyway, so I'd really rather not put any additional work into it.
+ print("Available x86 container images:\n") + print("\t" + "\n\t".join(native))
No tabs, please. Just use four spaces.
+ if cross: + print() + print("Available cross-compiler container images:\n") + print("\t" + "\n\t".join(cross))
Same here. With these small tweaks applied, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
This function checks whether there are any stale Docker images in the registry that can be purged. Since we're pulling available container images from our GitLab registry with the 'list-images' action, it could happen that we'd list old (already unsupported) images and make them available for the user to consume and run a build in them. Naturally, the build will most likely fail leaving the user confused. Signed-off-by: Erik Skultety <eskultet@redhat.com> --- ci/helper | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++- ci/util.py | 13 +++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/ci/helper b/ci/helper index 1b7675c58a..dbe3f80555 100755 --- a/ci/helper +++ b/ci/helper @@ -136,9 +136,16 @@ class Parser: refreshparser = subparsers.add_parser( "refresh", help="refresh data generated with lcitool", - parents=[lcitoolparser], + parents=[lcitoolparser, gitlabparser], formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) + refreshparser.add_argument( + "--check-stale", + action="store", + choices=["yes", "no"], + default="yes", + help="check for existence of stale images on the GitLab instance" + ) refreshparser.set_defaults(func=Application.action_refresh) def parse(self): @@ -286,10 +293,55 @@ class Application: print("Available cross-compiler container images:\n") print("\t" + "\n\t".join(cross)) + def _list_stale_images(self): + # get list of hosts supported by lcitool + lcitool_hosts = self.lcitool_get_hosts() + + # get images from gitlab registry + registry_uri = util.get_registry_uri(self.args.namespace, + self.args.gitlab_uri) + images = util.get_registry_images(registry_uri) + + # extract distro names from the list of registry images + registry_distros = [util.get_image_distro(i["name"]) for i in images] + + # check for stale images in GitLab registry + stale = set(registry_distros) - set(lcitool_hosts) + if stale: + stale_images = {} + for item in stale: + for img in images: + if item in img["name"]: + stale_images[img["name"]] = img["id"] + + return stale_images + def action_refresh(self): + # refresh Dockerfiles and vars files self.refresh_containers() self.refresh_cirrus() + # check for stale images + if self.args.check_stale == "yes" and not self.args.quiet: + namespace = self.args.namespace + gitlab_uri = self.args.gitlab_uri + + stale_images = self._list_stale_images() + if stale_images: + spacing = "\n" + 4 * " " + stale_fmt = [f"{k}: {v}" for k, v in stale_images.items()] + + print(f""" +The following images are stale and can be purged from the registry: +{spacing + spacing.join(stale_fmt)} + +You can remove the above images over the API with the following code snippet: + + $ for image_id in {' '.join([str(id) for id in stale_images.values()])}; do + curl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \\ + {util.get_registry_uri(namespace, gitlab_uri)}/$image_id + done""") + def run(self): self.args.func(self) diff --git a/ci/util.py b/ci/util.py index 8a2d6d8f47..f50c0e6100 100644 --- a/ci/util.py +++ b/ci/util.py @@ -37,3 +37,16 @@ def get_registry_images(uri: str) -> Dict[str, str]: # read the HTTP response and load the JSON part of it return json.loads(r.read().decode()) + + +def get_image_distro(image_name: str): + name_prefix = "ci-" + name_suffix = "-cross-" + + distro = image_name[len(name_prefix):] + + index = distro.find(name_suffix) + if index > 0: + distro = distro[:index] + + return distro -- 2.29.2
On Tue, 2021-03-16 at 18:33 +0100, Erik Skultety wrote:
+++ b/ci/helper + def _list_stale_images(self):
So far we have not used "_" as the prefix for any method name. Do we want to start doing that? What would be the criteria?
+ # check for stale images in GitLab registry + stale = set(registry_distros) - set(lcitool_hosts) + if stale: + stale_images = {} + for item in stale: + for img in images: + if item in img["name"]:
This should be if img["name"].startswith(item)
def action_refresh(self): + # refresh Dockerfiles and vars files self.refresh_containers() self.refresh_cirrus()
+ # check for stale images + if self.args.check_stale == "yes" and not self.args.quiet:
Can you please move the logic below into a separate function, similar to the refresh_containers() and refresh_cirrus() functions? To keep the entry point smaller.
+ stale_images = self._list_stale_images() + if stale_images: + spacing = "\n" + 4 * " "
I'm not convinced "\n" + 4 * " " is much better than "\n " so I'd probably go with the latter.
+ stale_fmt = [f"{k}: {v}" for k, v in stale_images.items()]
f"{k} ({v})" would work better here.
+ print(f""" +The following images are stale and can be purged from the registry: +{spacing + spacing.join(stale_fmt)} + +You can remove the above images over the API with the following code snippet: + + $ for image_id in {' '.join([str(id) for id in stale_images.values()])}; do + curl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \\ + {util.get_registry_uri(namespace, gitlab_uri)}/$image_id + done""")
I'm not a fan of the code snippets embedded inside the f-string... I'd rather do something like stale_details = spacing.join(stale_fmt) stale_ids = ' '.join([str(id) for id in stale_images.values()]) registry_uri = util.get_registry_uri(namespace, gitlab_uri) and then perform trivial variable substitution in the f-string. Looking at registry_uri specifically, I think it would make sense to rework list_stale_images() so that you pass the URI in, same as is already the case for get_registry_images(), to avoid having to produce it twice... And at that point, you might as well move the function to util.py and change its signature to get_registry_stale_images(uri, current_images) Another thing I'm not a fan of is having the message start at column zero instead of being aligned along with the rest of the code... textwrap.dedent() could help here, although it'd be a little annoying to use because we want the image details to be indented and we can't know the correct indent in advance, so we'd have to do a two-step process along the lines of msg = textwrap.dedent(f""" The following images are stale and can be purged from the registry: STALE_DETAILS ... """) print(msg.replace("STALE_DETAILS", stale_details)) A bit more cumbersome, but still preferable when it comes to readability IMHO. One last thing: we should end this message with something along the lines of You can generate a personal access token here: {self.args.gitlab_uri}/-/profile/personal_access_tokens
+++ b/ci/util.py +def get_image_distro(image_name: str):
Missing '-> str' here, I think. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Mar 17, 2021 at 04:57:01PM +0100, Andrea Bolognani wrote:
On Tue, 2021-03-16 at 18:33 +0100, Erik Skultety wrote:
+++ b/ci/helper + def _list_stale_images(self):
So far we have not used "_" as the prefix for any method name. Do we want to start doing that? What would be the criteria?
The criteria would be the common Python practice with naming class methods - signal that a method is private - an internal helper if you will - by prefixing it with an underscore.
+ # check for stale images in GitLab registry + stale = set(registry_distros) - set(lcitool_hosts) + if stale: + stale_images = {} + for item in stale: + for img in images: + if item in img["name"]:
This should be
if img["name"].startswith(item)
Unfortunately it cannot (although I'd like it to). 'item' in this case will be equal to what lcitool returns, so e.g. "debian-10", while the image names in GitLab are all named "ci-<distro>-<cross_arch>?"
def action_refresh(self): + # refresh Dockerfiles and vars files self.refresh_containers() self.refresh_cirrus()
+ # check for stale images + if self.args.check_stale == "yes" and not self.args.quiet:
Can you please move the logic below into a separate function, similar to the refresh_containers() and refresh_cirrus() functions? To keep the entry point smaller.
Sure, no problem.
+ stale_images = self._list_stale_images() + if stale_images: + spacing = "\n" + 4 * " "
I'm not convinced
"\n" + 4 * " "
is much better than
"\n "
Optically, it tells you exactly how many spaces there are without you moving the cursor.
so I'd probably go with the latter.
+ stale_fmt = [f"{k}: {v}" for k, v in stale_images.items()]
f"{k} ({v})" would work better here.
Okay...
+ print(f""" +The following images are stale and can be purged from the registry: +{spacing + spacing.join(stale_fmt)} + +You can remove the above images over the API with the following code snippet: + + $ for image_id in {' '.join([str(id) for id in stale_images.values()])}; do + curl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \\ + {util.get_registry_uri(namespace, gitlab_uri)}/$image_id + done""")
I'm not a fan of the code snippets embedded inside the f-string... I'd rather do something like
stale_details = spacing.join(stale_fmt) stale_ids = ' '.join([str(id) for id in stale_images.values()]) registry_uri = util.get_registry_uri(namespace, gitlab_uri)
Okay, I was out of ideas for descriptive variable names, but I won't argue that ^this is actually better.
and then perform trivial variable substitution in the f-string.
Looking at registry_uri specifically, I think it would make sense to rework list_stale_images() so that you pass the URI in, same as is already the case for get_registry_images(), to avoid having to produce it twice... And at that point, you might as well move the function to util.py and change its signature to
get_registry_stale_images(uri, current_images)
Another thing I'm not a fan of is having the message start at column zero instead of being aligned along with the rest of the code... textwrap.dedent() could help here, although it'd be a little annoying to use because we want the image details to be indented and we can't know the correct indent in advance, so we'd have to do a two-step process along the lines of
msg = textwrap.dedent(f""" The following images are stale and can be purged from the registry:
STALE_DETAILS
... """) print(msg.replace("STALE_DETAILS", stale_details))
Not sure I like ^this added complexity just to keep the code consistently indented. I fear non-existent indentation for verbatim strings """ which are meant to end up in the console is quite common. Last resort solution - if there happens to be more than a single occurrence of such hideous unindented verbatim string, we could consider dumping them into a separate file full of strings and just reference the one we need and dump data into them. That would however require usage of the older ".format()" practice as f-strings are evaluated in place.
A bit more cumbersome, but still preferable when it comes to readability IMHO.
One last thing: we should end this message with something along the lines of
You can generate a personal access token here:
{self.args.gitlab_uri}/-/profile/personal_access_tokens
Sure, I'll add it.
+++ b/ci/util.py +def get_image_distro(image_name: str):
Missing '-> str' here, I think.
Thanks, Erik
...
Another thing I'm not a fan of is having the message start at column zero instead of being aligned along with the rest of the code... textwrap.dedent() could help here, although it'd be a little annoying to use because we want the image details to be indented and we can't know the correct indent in advance, so we'd have to do a two-step process along the lines of
msg = textwrap.dedent(f""" The following images are stale and can be purged from the registry:
STALE_DETAILS
... """) print(msg.replace("STALE_DETAILS", stale_details))
A bit more cumbersome, but still preferable when it comes to readability IMHO.
So, I figured it would be more convenient to send a new revision rather than continue the discussion here, so basically apart from ^this last request for using textwrap.dedent I incorporated all your comments and we can discuss the text wrapping further in there. [v3] https://listman.redhat.com/archives/libvir-list/2021-March/msg00892.html Regards, Erik
participants (2)
-
Andrea Bolognani -
Erik Skultety