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