Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 26, 2024

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 rvagg requested review from galargh, masih and Stebalien March 26, 2024 10:41
Base automatically changed from ipdx-gha-test to master March 26, 2024 15:06
@rvagg rvagg force-pushed the rvagg/itest-config-inline branch from 29cd9c4 to dc6fc2a Compare March 28, 2024 06:42
@galargh
Copy link
Contributor

galargh commented Mar 28, 2024

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.

@rvagg
Copy link
Member Author

rvagg commented Apr 2, 2024

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

@Stebalien
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants