[libvirt PATCH v2 0/6] ci: helper: Rewrite image listing in Python and add stale Docker image checker
6:32 p.m.
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
6:32 p.m.
New subject: [libvirt PATCH v2 1/6] ci: Makefile: Specify a help target to replace ci-help
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(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(a)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
11:48 a.m.
New subject: [libvirt PATCH v2 1/6] ci: Makefile: Specify a help target to replace ci-help
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(a)redhat.com>
Reviewed-by: Andrea Bolognani <abologna(a)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
6:32 p.m.
New subject: [libvirt PATCH v2 2/6] ci: helper: Use the ArgumentDefaultsHelpFormatter help formatter
This help formatter class reports the defaults we use for options
taking an argument.
Signed-off-by: Erik Skultety <eskultet(a)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
11:52 a.m.
New subject: [libvirt PATCH v2 2/6] ci: helper: Use the ArgumentDefaultsHelpFormatter help formatter
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(a)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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
12:10 p.m.
New subject: [libvirt PATCH v2 2/6] ci: helper: Use the ArgumentDefaultsHelpFormatter help formatter
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(a)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(a)redhat.com>
Thanks,
Erik
6:32 p.m.
New subject: [libvirt PATCH v2 3/6] ci: helper: Introduce --quiet
Offer an option to silence all output to stdout, e.g. when generating
dockerfiles/varsfiles.
Signed-off-by: Erik Skultety <eskultet(a)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
11:54 a.m.
New subject: [libvirt PATCH v2 3/6] ci: helper: Introduce --quiet
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
2:31 p.m.
New subject: [libvirt PATCH v2 3/6] ci: helper: Introduce --quiet
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
6:32 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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(a)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
12:02 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
12:12 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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
12:14 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
5:06 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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
6:05 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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
6:11 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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
6:40 p.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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/
10:41 a.m.
New subject: [libvirt PATCH v2 4/6] ci: Introduce a util module
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
6:32 p.m.
New subject: [libvirt PATCH v2 5/6] ci: helper: Rewrite image listing to Python
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(a)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(a){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_page=100"
\
- | 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
12:09 p.m.
New subject: [libvirt PATCH v2 5/6] ci: helper: Rewrite image listing to Python
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(a)redhat.com>
--
Andrea Bolognani / Red Hat / Virtualization
6:33 p.m.
New subject: [libvirt PATCH v2 6/6] ci: util: Add a registry checker for stale images
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(a)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
4:57 p.m.
New subject: [libvirt PATCH v2 6/6] ci: util: Add a registry checker for stale images
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
6:24 p.m.
New subject: [libvirt PATCH v2 6/6] ci: util: Add a registry checker for stale images
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
9:12 a.m.
New subject: [libvirt PATCH v2 6/6] ci: util: Add a registry checker for stale 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.
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
1283
days inactive
1285
days old
23 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety