lotus icon indicating copy to clipboard operation
lotus copied to clipboard

ci: configure itests in build constraints, detect in Actions script

Open rvagg opened this issue 10 months ago • 3 comments

This is a stacked PR against #11762 and I'm exploring ways to configure itests inline, mainly because I'd like to not restrict this to the CI config, but also make it available in other ways, such as in the Makefile, as discussed in #11780. I can also imagine this being used to build some kind of fancy Docker or k8s config to do something similar, particularly as we get YugabyteDB involved.

A make itests would ideally not copy config that exists in the Actions config already (adding a new itest shouldn't involve wiring it up in multiple places—ideally no places but the itest itself!). It's also not really practical to be parsing yaml from make. So my thought here is to just use Go build constraints since they are an existing annotation pattern already. You could even use them to run them as a group with go test.

rvagg avatar Mar 26 '24 10:03 rvagg

One reservation I have about this approach concerns defining the GitHub Runners requirements in the tests themselves. In my opinion, this would needlessly expose CI implementation details to the integration test creators. For example, right now, we're using xlarge runners for a number of tests, but it's not because these tests need to run on these machines. It's only because there is a limited number of hosted runners available - 60 - and we would exhaust that limit with a single workflow run. These tests would be just fine running on hosted infrastructure. In fact, once we deprecate CircleCI, I think it'd be neat to try randomly assigning half the tests (that do not need 2xlarge or 4xlarge runners) to xlarge runners and the other half to hosted infrastructure. That way, we wouldn't maintain a list of tests in the workflow at all.

As for the requirements other than runner type - parameters, yugabytedb, etc. - it does make sense to me. Of course, it's up to the team to decide whether it works for everyone, but I quite like the approach myself :)

Would you mind if we hold off merging this until we deprecate CircleCI (expected 1-2 weeks)? We'd like to minimise the changes made to the GHA setup in the verification period to be able to compare the options better.

galargh avatar Mar 28 '24 12:03 galargh

good feedback @galargh, the runner size is quite leaky and it's good to hear that it's not specifically about the tests but the resources, I can adjust to that.

I'll hold off on taking this further, it's not urgent, I'd just like to get toward a better local way of running some of these that doesn't involve duplicating information we have in CI config

rvagg avatar Apr 02 '24 01:04 rvagg

I think the first step here is to start trying to reduce the number of tests that actually need custom runners. If we can get it down to a reasonable number, putting those exceptions in the CI config isn't terrible.

Stebalien avatar Apr 03 '24 20:04 Stebalien