[libvirt PATCH v3 0/6] ci: helper: Rewrite image listing in Python and add stale Docker image checker
3:09 a.m.
since v2:
- dropped original patch 1 renaming ci-help to help in Makefile
- changed the --quiet cmdline option to be tied to the refresh cmd only
- urrlib imports fixed
- improved on Python type hinting
- improved util functions docstrings
- replaced all tabs in formatted strings with 4 spaces
- moved the list_stale_images method to util as requested
- renamed variables in list_stale_images so that it's more clear at first
glance what they actually hold
- added a hint on where GitLab personal tokens should be generated
- added a patch converting private method naming to reflect the common
Python practice (I did it in a separate patch so that if this is not
accepted I only need to ditch a single patch with no further changes)
v1: https://listman.redhat.com/archives/libvir-list/2021-February/msg00641.ht=
ml
v2: https://listman.redhat.com/archives/libvir-list/2021-March/msg00789.html
EXAMPLE OUTPUT OF THE REGISTRY CHECKER:
The following images can be purged from the registry:
ci-debian-9: (ID: 1154661)
ci-debian-9-cross-aarch64: (ID: 1154667)
ci-debian-9-cross-mipsel: (ID: 1154669)
ci-debian-9-cross-armv7l: (ID: 1154671)
ci-debian-9-cross-armv6l: (ID: 1154676)
ci-debian-9-cross-mips: (ID: 1154678)
ci-debian-9-cross-ppc64le: (ID: 1154682)
ci-debian-9-cross-s390x: (ID: 1154683)
ci-debian-9-cross-mips64el: (ID: 1154686)
ci-fedora-31: (ID: 1154687)
ci-opensuse-151: (ID: 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
You can generate a personal access token here:
https://gitlab.com/-/profile/personal_access_tokens
Erik Skultety (6):
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: helper: Convert private methods to Python common naming practice
ci/Makefile | 12 ----
ci/helper | 162 ++++++++++++++++++++++++++++++++++++----------
ci/list-images.sh | 14 ----
ci/util.py | 92 ++++++++++++++++++++++++++
4 files changed, 219 insertions(+), 61 deletions(-)
delete mode 100644 ci/list-images.sh
create mode 100644 ci/util.py
--=20
2.29.2
3:09 a.m.
New subject: [libvirt PATCH v3 1/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>
Reviewed-by: Andrea Bolognani <abologna(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
3:09 a.m.
New subject: [libvirt PATCH v3 2/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/helper | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/ci/helper b/ci/helper
index 8f34f4b59d..53baef3cb1 100755
--- a/ci/helper
+++ b/ci/helper
@@ -116,6 +116,12 @@ class Parser:
parents=[lcitoolparser],
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
+ refreshparser.add_argument(
+ "--quiet",
+ action="store_true",
+ default=False,
+ help="refresh data silently"
+ )
refreshparser.set_defaults(func=Application.action_refresh)
def parse(self):
@@ -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
3:09 a.m.
New subject: [libvirt PATCH v3 3/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/util.py | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 ci/util.py
diff --git a/ci/util.py b/ci/util.py
new file mode 100644
index 0000000000..f9f8320276
--- /dev/null
+++ b/ci/util.py
@@ -0,0 +1,40 @@
+import json
+import urllib.request
+import urllib.parse
+
+from typing import Dict, List
+
+
+def get_registry_uri(namespace: str,
+ gitlab_uri: str = "https://gitlab.com") -> str:
+ """
+ Construct a v4 API URI pointing the namespaced project's image registry.
+
+ :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 = urllib.parse.quote_plus(namespace)
+
+ project_uri = f"{gitlab_uri}/api/v4/projects/{namespace_urlenc}"
+
+ uri = project_uri + "/registry/repositories"
+ return uri
+
+
+def get_registry_images(uri: str) -> List[Dict]:
+ """
+ List all container images that are currently available in the given GitLab
+ project.
+
+ :param uri: URI pointing to a GitLab instance's image registry
+ :return: list of container image names
+ """
+
+ r = urllib.request.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
3:09 a.m.
New subject: [libvirt PATCH v3 4/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>
Reviewed-by: Andrea Bolognani <abologna(a)redhat.com>
---
ci/Makefile | 12 ------------
ci/helper | 38 +++++++++++++++++++++++++++++++++++++-
ci/list-images.sh | 14 --------------
3 files changed, 37 insertions(+), 27 deletions(-)
delete mode 100644 ci/list-images.sh
diff --git a/ci/Makefile b/ci/Makefile
index 72f5bda942..02ce0df7ee 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -211,17 +211,6 @@ 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-help:
@echo
@echo
@@ -241,7 +230,6 @@ ci-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 " ci-help - show this help message"
@echo
@echo "Available make variables:"
diff --git a/ci/helper b/ci/helper
index 53baef3cb1..31cf72fbdf 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()
subparsers = self.parser.add_subparsers(
@@ -105,6 +121,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,26 @@ 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]
+
+ 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))
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
3:09 a.m.
New subject: [libvirt PATCH v3 5/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 | 45 ++++++++++++++++++++++++++++++++++++++++++++-
ci/util.py | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/ci/helper b/ci/helper
index 31cf72fbdf..b5255db835 100755
--- a/ci/helper
+++ b/ci/helper
@@ -130,7 +130,7 @@ 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(
@@ -139,6 +139,13 @@ class Parser:
default=False,
help="refresh data silently"
)
+ 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):
@@ -287,10 +294,46 @@ class Application:
print("Available cross-compiler container images:\n")
print(spacing + ("\n" + spacing).join(cross))
+ def check_stale_images(self):
+ if self.args.check_stale != "yes" or self.args.quiet:
+ return
+
+ namespace = self.args.namespace
+ gitlab_uri = self.args.gitlab_uri
+ registry_uri = util.get_registry_uri(namespace, gitlab_uri)
+ lcitool_hosts = self.lcitool_get_hosts()
+
+ stale_images = util.get_registry_stale_images(registry_uri,
+ lcitool_hosts)
+ if stale_images:
+ spacing = "\n" + 4 * " "
+ stale_fmt = [f"{k} (ID: {v})" for k, v in stale_images.items()]
+ stale_details = spacing + spacing.join(stale_fmt)
+ stale_ids = ' '.join([str(id) for id in stale_images.values()])
+ registry_uri = util.get_registry_uri(namespace, gitlab_uri)
+
+ print(f"""
+The following images are stale and can be purged from the registry:
+{stale_details}
+
+You can remove the above images over the API with the following code snippet:
+
+ $ for image_id in {stale_ids}; do \\
+ curl --request DELETE --header "PRIVATE-TOKEN: <access_token>"
\\
+ {registry_uri}/$image_id \\
+ done
+
+You can generate a personal access token here:
+ {gitlab_uri}/-/profile/personal_access_tokens""")
+
def action_refresh(self):
+ # refresh Dockerfiles and vars files
self.refresh_containers()
self.refresh_cirrus()
+ # check for stale images
+ self.check_stale_images()
+
def run(self):
self.args.func(self)
diff --git a/ci/util.py b/ci/util.py
index f9f8320276..d69c246872 100644
--- a/ci/util.py
+++ b/ci/util.py
@@ -38,3 +38,55 @@ def get_registry_images(uri: str) -> List[Dict]:
# read the HTTP response and load the JSON part of it
return json.loads(r.read().decode())
+
+
+def get_image_distro(image_name: str) -> str:
+ """
+ Extract the name of the distro in the GitLab image registry name, e.g.
+ ci-debian-9-cross-mipsel --> debian-9
+
+ :param image_name: name of the GitLab registry image
+ :return: distro name as a string
+ """
+ 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
+
+
+def get_registry_stale_images(registry_uri: str,
+ supported_distros: List[str]) -> Dict[str, int]:
+ """
+ Check the GitLab image registry for images that we no longer support and
+ which should be deleted.
+
+ :param uri: URI pointing to a GitLab instance's image registry
+ :param supported_distros: list of hosts supported by lcitool
+ :return: dictionary formatted as: {<gitlab_image_name>:
<gitlab_image_id>}
+ """
+
+ images = get_registry_images(registry_uri)
+
+ # extract distro names from the list of registry images
+ registry_distros = [get_image_distro(i["name"]) for i in images]
+
+ # - compare the distros powering the images in GitLab registry with
+ # the list of host available from lcitool
+ # - @unsupported is a set containing the distro names which we no longer
+ # support; we need to map these back to registry image names
+ unsupported = set(registry_distros) - set(supported_distros)
+ if unsupported:
+ stale_images = {}
+ for distro in unsupported:
+ for img in images:
+ # gitlab images are named like
"ci-<distro>-<cross_arch>?"
+ if distro in img["name"]:
+ stale_images[img["name"]] = img["id"]
+
+ return stale_images
--
2.29.2
5:54 a.m.
New subject: [libvirt PATCH v3 5/6] ci: util: Add a registry checker for stale images
On Thu, 2021-03-18 at 09:09 +0100, Erik Skultety wrote:
+ def check_stale_images(self):
+ if self.args.check_stale != "yes" or self.args.quiet:
+ return
I would prefer it if this check were to be performed in the
action_refresh() method to decide whether or not check_stale_images()
should be called.
+ print(f"""
+The following images are stale and can be purged from the registry:
+{stale_details}
+
+You can remove the above images over the API with the following code snippet:
"You can delete the images listed above using this shell snippet:"
+ $ for image_id in {stale_ids}; do \\
+ curl --request DELETE --header "PRIVATE-TOKEN: <access_token>"
\\
+ {registry_uri}/$image_id \\
+ done
This will not result in a working shell snippet being displayed. What
you want is
f"""
$ for image_id in {stale_ids}; do
curl --request DELETE --header "PRIVATE-TOKEN: <access_token>"
\\
{registry_uri}/$image_id;
done
"""
+
+You can generate a personal access token here:
+ {gitlab_uri}/-/profile/personal_access_tokens""")
Empty line before the URL for readability.
I still think that we should use textwrap.dedent(), as it makes the
script much more readable at the cost of a single additional
string.replace() call:
if stale_images:
spacing = "\n" + 4 * " "
stale_fmt = [f"{k} (ID: {v})" for k, v in stale_images.items()]
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)
msg = textwrap.dedent(f"""
The following images are stale and can be purged from the registry:
STALE_DETAILS
You can delete the images listed above using this shell snippet:
$ for image_id in {stale_ids}; do
curl --request DELETE --header "PRIVATE-TOKEN:
<access_token>" \\
{registry_uri}/$image_id; \\
done
You can generate a personal access token here:
{gitlab_uri}/-/profile/personal_access_tokens
""")
print(msg.replace("STALE_DETAILS", stale_details))
def action_refresh(self):
+ # refresh Dockerfiles and vars files
self.refresh_containers()
self.refresh_cirrus()
+ # check for stale images
+ self.check_stale_images()
The method names are really quite self-explanatory, so the comments
you're introducing don't add much IMHO... I'd say leave them out.
+def get_registry_stale_images(registry_uri: str,
+ supported_distros: List[str]) -> Dict[str, int]:
+ """
+ Check the GitLab image registry for images that we no longer support and
+ which should be deleted.
+
+ :param uri: URI pointing to a GitLab instance's image registry
+ :param supported_distros: list of hosts supported by lcitool
+ :return: dictionary formatted as: {<gitlab_image_name>:
<gitlab_image_id>}
+ """
+
+ images = get_registry_images(registry_uri)
+
+ # extract distro names from the list of registry images
+ registry_distros = [get_image_distro(i["name"]) for i in images]
+
+ # - compare the distros powering the images in GitLab registry with
+ # the list of host available from lcitool
+ # - @unsupported is a set containing the distro names which we no longer
+ # support; we need to map these back to registry image names
+ unsupported = set(registry_distros) - set(supported_distros)
+ if unsupported:
+ stale_images = {}
+ for distro in unsupported:
+ for img in images:
+ # gitlab images are named like
"ci-<distro>-<cross_arch>?"
+ if distro in img["name"]:
+ stale_images[img["name"]] = img["id"]
+
+ return stale_images
As far as I can tell, this can be achieved in a much more
straightforward way with
def get_registry_stale_images(registry_uri, supported_distros):
images = get_registry_images(registry_uri)
stale_images = {}
for img in images:
if get_image_distro(img["name"]) not in supported_distros:
stale_images[img["name"]] = img["id"]
return stale_images
At least from a quick test, the results appear to be the same. Am I
missing something?
--
Andrea Bolognani / Red Hat / Virtualization
11 a.m.
New subject: [libvirt PATCH v3 5/6] ci: util: Add a registry checker for stale images
...
> +def get_registry_stale_images(registry_uri: str,
> + supported_distros: List[str]) -> Dict[str, int]:
> + """
> + Check the GitLab image registry for images that we no longer support and
> + which should be deleted.
> +
> + :param uri: URI pointing to a GitLab instance's image registry
> + :param supported_distros: list of hosts supported by lcitool
> + :return: dictionary formatted as: {<gitlab_image_name>:
<gitlab_image_id>}
> + """
> +
> + images = get_registry_images(registry_uri)
> +
> + # extract distro names from the list of registry images
> + registry_distros = [get_image_distro(i["name"]) for i in images]
> +
> + # - compare the distros powering the images in GitLab registry with
> + # the list of host available from lcitool
> + # - @unsupported is a set containing the distro names which we no longer
> + # support; we need to map these back to registry image names
> + unsupported = set(registry_distros) - set(supported_distros)
> + if unsupported:
> + stale_images = {}
> + for distro in unsupported:
> + for img in images:
> + # gitlab images are named like
"ci-<distro>-<cross_arch>?"
> + if distro in img["name"]:
> + stale_images[img["name"]] = img["id"]
> +
> + return stale_images
As far as I can tell, this can be achieved in a much more
straightforward way with
def get_registry_stale_images(registry_uri, supported_distros):
images = get_registry_images(registry_uri)
stale_images = {}
for img in images:
if get_image_distro(img["name"]) not in supported_distros:
stale_images[img["name"]] = img["id"]
return stale_images
At least from a quick test, the results appear to be the same. Am I
missing something?
No, it works. I'll respin.
Erik
3:09 a.m.
New subject: [libvirt PATCH v3 6/6] ci: helper: Convert private methods to Python common naming practice
As documented at [1], the common practice wrt to private
attributes/methods naming is to prefix them with an underscore.
[1] https://docs.python.org/3/tutorial/classes.html#private-variables
Signed-off-by: Erik Skultety <eskultet(a)redhat.com>
---
ci/helper | 68 +++++++++++++++++++++++++++----------------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/ci/helper b/ci/helper
index b5255db835..3d2e0397f5 100755
--- a/ci/helper
+++ b/ci/helper
@@ -97,7 +97,7 @@ class Parser:
parents=[containerparser, mesonparser],
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
- buildparser.set_defaults(func=Application.action_build)
+ buildparser.set_defaults(func=Application._action_build)
# test action
testparser = subparsers.add_parser(
@@ -106,7 +106,7 @@ class Parser:
parents=[containerparser, mesonparser],
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
- testparser.set_defaults(func=Application.action_test)
+ testparser.set_defaults(func=Application._action_test)
# shell action
shellparser = subparsers.add_parser(
@@ -115,7 +115,7 @@ class Parser:
parents=[containerparser],
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
- shellparser.set_defaults(func=Application.action_shell)
+ shellparser.set_defaults(func=Application._action_shell)
# list-images action
listimagesparser = subparsers.add_parser(
@@ -124,7 +124,7 @@ class Parser:
parents=[gitlabparser],
formatter_class=argparse.ArgumentDefaultsHelpFormatter,
)
- listimagesparser.set_defaults(func=Application.action_list_images)
+ listimagesparser.set_defaults(func=Application._action_list_images)
# refresh action
refreshparser = subparsers.add_parser(
@@ -146,7 +146,7 @@ class Parser:
default="yes",
help="check for existence of stale images on the GitLab instance"
)
- refreshparser.set_defaults(func=Application.action_refresh)
+ refreshparser.set_defaults(func=Application._action_refresh)
def parse(self):
return self.parser.parse_args()
@@ -161,7 +161,7 @@ class Application:
if not shutil.which(self.args.lcitool):
sys.exit("error: 'lcitool' not installed")
- def make_run(self, target):
+ def _make_run(self, target):
args = [
"-C",
self.basedir,
@@ -185,15 +185,15 @@ class Application:
if pty.spawn(["make"] + args) != 0:
sys.exit("error: 'make' failed")
- def lcitool_run(self, args):
+ def _lcitool_run(self, args):
output = subprocess.check_output([self.args.lcitool] + args)
return output.decode("utf-8")
- def lcitool_get_hosts(self):
- output = self.lcitool_run(["hosts"])
+ def _lcitool_get_hosts(self):
+ output = self._lcitool_run(["hosts"])
return output.splitlines()
- def generate_dockerfile(self, host, cross=None):
+ def _generate_dockerfile(self, host, cross=None):
args = ["dockerfile", host, "libvirt"]
outdir = self.basedir.joinpath("containers")
outfile = f"ci-{host}.Dockerfile"
@@ -206,11 +206,11 @@ class Application:
if not self.args.quiet:
print(outpath)
- output = self.lcitool_run(args)
+ output = self._lcitool_run(args)
with open(outpath, "w") as f:
f.write(output)
- def generate_vars(self, host):
+ def _generate_vars(self, host):
args = ["variables", host, "libvirt"]
outdir = self.basedir.joinpath("cirrus")
outfile = f"{host}.vars"
@@ -219,11 +219,11 @@ class Application:
if not self.args.quiet:
print(outpath)
- output = self.lcitool_run(args)
+ output = self._lcitool_run(args)
with open(outpath, "w") as f:
f.write(output)
- def refresh_containers(self):
+ def _refresh_containers(self):
debian_cross = [
"aarch64",
"armv6l",
@@ -240,39 +240,39 @@ class Application:
"mingw64",
]
- for host in self.lcitool_get_hosts():
+ for host in self._lcitool_get_hosts():
if host.startswith("freebsd-") or
host.startswith("macos-"):
continue
- self.generate_dockerfile(host)
+ self._generate_dockerfile(host)
if host == "fedora-rawhide":
for cross in fedora_cross:
- self.generate_dockerfile(host, cross)
+ self._generate_dockerfile(host, cross)
if host.startswith("debian-"):
for cross in debian_cross:
if host == "debian-sid" and cross == "mips":
continue
- self.generate_dockerfile(host, cross)
+ self._generate_dockerfile(host, cross)
- def refresh_cirrus(self):
- for host in self.lcitool_get_hosts():
+ def _refresh_cirrus(self):
+ for host in self._lcitool_get_hosts():
if not (host.startswith("freebsd-") or
host.startswith("macos-")):
continue
- self.generate_vars(host)
+ self._generate_vars(host)
- def action_build(self):
- self.make_run(f"ci-build(a){self.args.target}")
+ def _action_build(self):
+ self._make_run(f"ci-build(a){self.args.target}")
- def action_test(self):
- self.make_run(f"ci-test(a){self.args.target}")
+ def _action_test(self):
+ self._make_run(f"ci-test(a){self.args.target}")
- def action_shell(self):
- self.make_run(f"ci-shell(a){self.args.target}")
+ def _action_shell(self):
+ self._make_run(f"ci-shell(a){self.args.target}")
- def action_list_images(self):
+ def _action_list_images(self):
registry_uri = util.get_registry_uri(self.args.namespace,
self.args.gitlab_uri)
images = util.get_registry_images(registry_uri)
@@ -294,14 +294,14 @@ class Application:
print("Available cross-compiler container images:\n")
print(spacing + ("\n" + spacing).join(cross))
- def check_stale_images(self):
+ def _check_stale_images(self):
if self.args.check_stale != "yes" or self.args.quiet:
return
namespace = self.args.namespace
gitlab_uri = self.args.gitlab_uri
registry_uri = util.get_registry_uri(namespace, gitlab_uri)
- lcitool_hosts = self.lcitool_get_hosts()
+ lcitool_hosts = self._lcitool_get_hosts()
stale_images = util.get_registry_stale_images(registry_uri,
lcitool_hosts)
@@ -326,13 +326,13 @@ You can remove the above images over the API with the following code
snippet:
You can generate a personal access token here:
{gitlab_uri}/-/profile/personal_access_tokens""")
- def action_refresh(self):
+ def _action_refresh(self):
# refresh Dockerfiles and vars files
- self.refresh_containers()
- self.refresh_cirrus()
+ self._refresh_containers()
+ self._refresh_cirrus()
# check for stale images
- self.check_stale_images()
+ self._check_stale_images()
def run(self):
self.args.func(self)
--
2.29.2
6:09 a.m.
New subject: [libvirt PATCH v3 6/6] ci: helper: Convert private methods to Python common naming practice
On Thu, 2021-03-18 at 09:09 +0100, Erik Skultety wrote:
As documented at [1], the common practice wrt to private
s/wrt to/with respect to/
def parse(self):
return self.parser.parse_args()
Shouldn't this change be applied to Parser.parser...
def run(self):
self.args.func(self)
... and Application.args too?
Everything else looks good.
--
Andrea Bolognani / Red Hat / Virtualization
11:08 a.m.
New subject: [libvirt PATCH v3 6/6] ci: helper: Convert private methods to Python common naming practice
On Thu, Mar 18, 2021 at 12:09:19PM +0100, Andrea Bolognani wrote:
On Thu, 2021-03-18 at 09:09 +0100, Erik Skultety wrote:
> As documented at [1], the common practice wrt to private
s/wrt to/with respect to/
> def parse(self):
> return self.parser.parse_args()
Shouldn't this change be applied to Parser.parser...
> def run(self):
> self.args.func(self)
... and Application.args too?
Everything else looks good.
Applied.
Erik
5:36 a.m.
New subject: [libvirt PATCH v3 6/6] ci: helper: Convert private methods to Python common naming practice
On Thu, Mar 18, 2021 at 12:09:19PM +0100, Andrea Bolognani wrote:
On Thu, 2021-03-18 at 09:09 +0100, Erik Skultety wrote:
> As documented at [1], the common practice wrt to private
s/wrt to/with respect to/
> def parse(self):
> return self.parser.parse_args()
Shouldn't this change be applied to Parser.parser...
> def run(self):
> self.args.func(self)
... and Application.args too?
Everything else looks good.
May I assume your R-b for this patch after having resolved this in v4?
Erik
5:42 a.m.
New subject: [libvirt PATCH v3 6/6] ci: helper: Convert private methods to Python common naming practice
On Fri, 2021-03-19 at 11:36 +0100, Erik Skultety wrote:
On Thu, Mar 18, 2021 at 12:09:19PM +0100, Andrea Bolognani wrote:
> On Thu, 2021-03-18 at 09:09 +0100, Erik Skultety wrote:
> > As documented at [1], the common practice wrt to private
>
> s/wrt to/with respect to/
>
> > def parse(self):
> > return self.parser.parse_args()
>
> Shouldn't this change be applied to Parser.parser...
>
> > def run(self):
> > self.args.func(self)
>
> ... and Application.args too?
>
> Everything else looks good.
May I assume your R-b for this patch after having resolved this in v4?
Sure thing! I thought I had sent an explicit one, but it looks like I
failed to do so after all O:-)
--
Andrea Bolognani / Red Hat / Virtualization
1335
days inactive
1336
days old
12 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety