[libvirt PATCH 4/4] ci: Introduce a new checker script 'check-registry.py'

[Andrea Bolognani]

On Wed, 2021-02-10 at 18:24 +0100, Erik Skultety wrote:
> +def get_undesirables(registry_distros_d, hosts_l):
> +    """ Returns a dictionary of 'id':'name' pairs of images that can be purged"""

Is the var_d and var_l a Python convention that I'm not aware of? I
don't think we're using it anywhere in either libvirt or libvirt-ci,
so I'd rather avoid it here as well.

I see you're using some type hints for the get_image_distro()
function, though. Can we perhaps have more of that?

Suggestion: "stale" is both shorter and harder to misspell than
"undesirable" :)

> +def main():
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("project_id",
> +                        help="GitLab project ID")

Is passing the project ID necessary? We're hardcoding the one for
libvirt/libvirt in util.py and we don't provide a way to override it
for ci-list-images, so I think this can be skipped.

Having a way for the developer to point to their own fork rather than
libvirt/libvirt makes sense, but in that case we probably also want
to make it so the user passes "myusername/libvirt" and we convert
that to a project ID using the GitLab API under the hood.

> +    parser.add_argument("--lcitool_path",
> +                        dest="lcitool",
> +                        metavar="PATH",
> +                        help="absolute path to lcitool, CWD is used if omitted")

Underscores in command line arguments are inconvenient and ugly, so
please don't use them. You can just call the option "--lcitool".

Regarding the documentation for the option, I'm not convinced it's
accurate: AFAICT shutil.which() will use $PATH, not $CWD, to look for
the named binary if an absolute path to it has not been provided.

> +    uri = util.get_registry_uri(util.PROJECT_ID)
> +    images_json = util.list_images(uri + "?per_page=100")

We have "?per_page=100" hardcoded in two places now, so it looks like
we can probably just fold its usage into list_images()?

> +        sys.exit(f"""
> +The following images can be purged from the registry:
> +
> +{undesirable_image_names}

I would like to see the image ID printed along with the image name,
so that I can tweak the shell snippet below to selectively remove
only *some* of the images.

> +You can remove the above images over the API with the following code snippet:
> +
> +\t$ for image_id in {undesirable_image_ids} \\
> +\t;do \\

Having the "do" on a separate line is weird.

Also please indent using either two or four spaces rather than tabs.


Aside from the (mostly minor) issues that I've pointed out, I think
these patches go in the right direction. However, reviewing them made
me wonder what the long-term status of these scripts should be.

Right now we have the following entry points:

  * a Makefile, which can be used to kick off local builds;

  * a couple of 'refresh' shell script that are used to regenerate
    contents derived from the libvirt-ci source of truth;

  * some Python bits that are used by both.

We also know that, at some point in the hopefully not too distant
future, we'll have the results of

  https://gitlab.com/libvirt/libvirt-ci/-/merge_requests/59

available, and that will allow us to turn the refresh scripts into
trivial wrappers.

I think it would make sense to converge towards having a single
Python based entry point, which serves as the counterpart of lcitool,
and which can be used to do all of the above.

We can even get there gradually: the first version of the Python
script could just call out to the Makefile to start builds, and then
we could reimplement the logic in Python at a later point in time.

What do you think?

-- 
Andrea Bolognani / Red Hat / Virtualization