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

[Erik Skultety]

On Wed, Mar 17, 2021 at 04:57:01PM +0100, Andrea Bolognani wrote:
> 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?

The criteria would be the common Python practice with naming class methods -
signal that a method is private - an internal helper if you will - by prefixing
it with an underscore.

> 
> > +        # 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)

Unfortunately it cannot (although I'd like it to). 'item' in this case will
be equal to what lcitool returns, so e.g. "debian-10", while the image names in
GitLab are all named "ci-<distro>-<cross_arch>?"

> 
> >      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.

Sure, no problem.

> 
> > +            stale_images = self._list_stale_images()
> > +            if stale_images:
> > +                spacing = "\n" + 4 * " "
> 
> I'm not convinced
> 
>   "\n" + 4 * " "
> 
> is much better than
> 
>   "\n    "

Optically, it tells you exactly how many spaces there are without you moving
the cursor.

> 
> 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.

Okay...

> 
> > +                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)

Okay, I was out of ideas for descriptive variable names, but I won't argue that
^this is actually better.

> 
> 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))

Not sure I like ^this added complexity just to keep the code consistently
indented. I fear non-existent indentation for verbatim strings """ which are
meant to end up in the console is quite common. Last resort solution - if
there happens to be more than a single occurrence of such hideous unindented
verbatim string, we could consider dumping them into a separate file full of
strings and just reference the one we need and dump data into them. That would
however require usage of the older ".format()" practice as f-strings are
evaluated in place.

> 
> 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

Sure, I'll add it.

> 
> > +++ b/ci/util.py
> > +def get_image_distro(image_name: str):
> 
> Missing '-> str' here, I think.

Thanks,
Erik