Re: [libvirt PATCH v2 6/6] ci: util: Add a registry checker for stale images

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux