On Tue, 2021-03-16 at 18:33 +0100, Erik Skultety wrote:
+++ b/ci/helper + def _list_stale_images(self):
So far we have not used "_" as the prefix for any method name. Do we want to start doing that? What would be the criteria?
+ # check for stale images in GitLab registry + stale = set(registry_distros) - set(lcitool_hosts) + if stale: + stale_images = {} + for item in stale: + for img in images: + if item in img["name"]:
This should be if img["name"].startswith(item)
def action_refresh(self): + # refresh Dockerfiles and vars files self.refresh_containers() self.refresh_cirrus()
+ # check for stale images + if self.args.check_stale == "yes" and not self.args.quiet:
Can you please move the logic below into a separate function, similar to the refresh_containers() and refresh_cirrus() functions? To keep the entry point smaller.
+ stale_images = self._list_stale_images() + if stale_images: + spacing = "\n" + 4 * " "
I'm not convinced "\n" + 4 * " " is much better than "\n " so I'd probably go with the latter.
+ stale_fmt = [f"{k}: {v}" for k, v in stale_images.items()]
f"{k} ({v})" would work better here.
+ print(f""" +The following images are stale and can be purged from the registry: +{spacing + spacing.join(stale_fmt)} + +You can remove the above images over the API with the following code snippet: + + $ for image_id in {' '.join([str(id) for id in stale_images.values()])}; do + curl --request DELETE --header "PRIVATE-TOKEN: <access_token>" \\ + {util.get_registry_uri(namespace, gitlab_uri)}/$image_id + done""")
I'm not a fan of the code snippets embedded inside the f-string... I'd rather do something like 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) and then perform trivial variable substitution in the f-string. Looking at registry_uri specifically, I think it would make sense to rework list_stale_images() so that you pass the URI in, same as is already the case for get_registry_images(), to avoid having to produce it twice... And at that point, you might as well move the function to util.py and change its signature to get_registry_stale_images(uri, current_images) Another thing I'm not a fan of is having the message start at column zero instead of being aligned along with the rest of the code... textwrap.dedent() could help here, although it'd be a little annoying to use because we want the image details to be indented and we can't know the correct indent in advance, so we'd have to do a two-step process along the lines of msg = textwrap.dedent(f""" The following images are stale and can be purged from the registry: STALE_DETAILS ... """) print(msg.replace("STALE_DETAILS", stale_details)) A bit more cumbersome, but still preferable when it comes to readability IMHO. One last thing: we should end this message with something along the lines of You can generate a personal access token here: {self.args.gitlab_uri}/-/profile/personal_access_tokens
+++ b/ci/util.py +def get_image_distro(image_name: str):
Missing '-> str' here, I think. -- Andrea Bolognani / Red Hat / Virtualization