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
