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