On Thu, Aug 31, 2023 at 06:28:16PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 25, 2023 at 07:55:08PM +0200, Erik Skultety wrote:
> > Technically a v2 of:
> > https://listman.redhat.com/archives/libvir-list/2023-February/237552.html
> >
> > However, the approach here is slightly different and what that series said
> > about migration to lcitool container executions as a replacement for
> > ci/Makefile is actually done here. One of the core problems of the above
> > pointed out in review was that more Shell logic was introduced including CLI
> > parsing, conditional executions, etc. which we fought hard to get rid of in the
> > past. I reworked the Shell functions quite a bit and dropped whatever extra
> > Shell logic the original series added.
> > Obviously we can't get rid of Shell completely because of .gitlab-ci.yml and so
> > I merely extracted the recipes into functions which are then sourced as
> > ci/build.sh and executed. Now, that on its own would hide the actual commands
> > being run in the GitLab job log, so before any command is actually executed, it
> > is formatted with a color sequence so we don't miss that information as that
> > would be a regression to the status quo.
> >
> > Lastly, this series then takes the effort inside the ci/build.sh script and
> > basically mirrors whatever GitLab would do to run a job inside a local
> > container which is executed by lcitool (yes, we already have that capability).
> >
> > Please give this a try and I'm already looking forward to comments as I'd like
> > to expand this effort to local VM executions running the TCK integration tests,
> > so this series is quite important in that regard.
>
> Overall I'm fine with what's proposed here.
>
> Two general thoughts
>
> * ci/Makefile appears pretty much redundant - ci/helper can provide
> the same level of functionality AFAICT, and it'd be nice to kill
> an outstanding usage of 'make' given our goal to standardize on
> meson + python
Huh, the fact that removal of Makefile isn't part of this series is a mistake
on my side - I worked on this on 2 parallel branches trying out 2 slightly
different approaches. I did drop it on one branch but not the other which I
ultimately decided to go with. Since I'll be sending a v2, I'll add that patch.
>
> * ci/helper looks almost entirely independent of libvirt, aside
> from the list of 'choices' for the --job arg, and the --namespace
> arg default value, it would work with any virt project we have if
> the project created its own ci/build.sh file
>
> Can we fold all its logic into lcitool, and just have that as
> the entrypoint ? In ci/manifest.yml we can get the project
> namespace, and we could possibly just extra the commands by
> crudely regex matching 'ci/build.sh' content against the
> pattern '^run_.*\(\)$ {'
Technically we could. Extracting the code and injecting it into lcitool is not
a problem, in fact, it would be quite straight forward. The problem is
designing a CLI interface that would make sense for the use case without
breaking the existing one too much. Ideally by introducing just a bunch of
optional args which I don't think is very realistic. Since that will require
thorough thinking and designing I did not want to dive right into that as I
wasn't even sure whether I'd be able to push this conversion through upstream.
>
>
> The removal of ci/Makefil feels like it could be done in this series,
> but its fine if the ci/helper suggestion is left as separate future
> work.
Yeah, like I said above, incorporating ci/helper into lcitool is likely going
to be again one of the bigger overhauls so that will be a project on its own.
Thanks for the comments, much appreciated.
Erik