Re: ci: Allow running mingw tests by default via environment variable

Hi,

Thank you for working on this!

On Sat, 13 Apr 2024 at 05:12, Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> We have CI support for mingw, but don't run the task by default, as it eats up
> precious CI credits.  However, for cfbot we are using custom compute resources
> (currently donated by google), so we can afford to run the mingw tests. Right
> now that'd require cfbot to patch .cirrus.tasks.yml.

And I think mingw ends up not running most of the time. +1 to running
it as default at least on cfbot. Also, this gives people a chance to
run mingw as default on their personal repositories (which I would
like to run).

> While one can manually trigger manual task in one one's own repo, most won't
> have the permissions to do so for cfbot.
>
>
> I propose that we instead run the task automatically if
> $REPO_MINGW_TRIGGER_BY_DEFAULT is set, typically in cirrus' per-repository
> configuration.
>
> Unfortunately that's somewhat awkward to do in the cirrus-ci yaml
> configuration, the set of conditional expressions supported is very
> simplistic.
>
> To deal with that, I extended .cirrus.star to compute the required environment
> variable. If $REPO_MINGW_TRIGGER_BY_DEFAULT is set, CI_MINGW_TRIGGER_TYPE is
> set to 'automatic', if not it's 'manual'.
>

Changes look good to me. My only complaint could be using only 'true'
for the REPO_MINGW_TRIGGER_BY_DEFAULT, not a possible list of values
but this is not important.

> We've also talked in other threads about adding CI support for
> 1) windows, building with visual studio
> 2) linux, with musl libc
> 3) free/netbsd
>
> That becomes more enticing, if we can enable them by default on cfbot but not
> elsewhere. With this change, it'd be easy to add further variables to control
> such future tasks.

I agree.

> I also attached a second commit, that makes the "code" dealing with ci-os-only
> in .cirrus.tasks.yml simpler. While I think it does nicely simplify
> .cirrus.tasks.yml, overall it adds lines, possibly making this not worth it.
> I'm somewhat on the fence.
>
>
> Thoughts?

Although it adds more lines, this makes the .cirrus.tasks.yml file
more readable and understandable which is more important in my
opinion.

> On the code level, I thought if it'd be good to have a common prefix for all
> the automatically set variables. Right now that's CI_, but I'm not at all
> wedded to that.

I agree with your thoughts and CI_ prefix.

I tested both patches and they work as expected.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft