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
