-
Notifications
You must be signed in to change notification settings - Fork 283
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
Run ARM64 tests in CI #94
Conversation
|
||
set -eu | ||
set -o pipefail | ||
|
||
qemu_arch="${QEMU_ARCH:-x86_64}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could make this a positional argument to run-tests.sh
. No strong preference either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Env variable seems good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
uses: actions/setup-go@v5 | ||
with: | ||
go-version: ${{matrix.go}} | ||
go-version: stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Using stable
as version for Go comes with the risk of breaking CI if there is an unexpected change in Go. E.g. there could be changes similar to golang/go#67401, that could break assumptions.
Manually updating the Go version allows to make sure CI does not break on events, that are happening outside of the scope and control of this repo/maintainership.
Ideally, this action should receive some version information as argument. Using $matrix.go
here assumes that in the executed job matrix.go
is defined and makes it hard to follow/review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Using stable as version for Go comes with the risk of breaking CI if there is an unexpected change in Go. E.g. there could be changes similar to golang/go#67401, that could break assumptions.
I'm merely mirroring the previous logic here, but I'd also be OK with hardcoding a particular Go version globally. IMO we should just have one global Go version for all CI workflows and adding it as an argument would force us to duplicate the version everywhere.
Using $matrix.go here assumes that in the executed job matrix.go is defined and makes it hard to follow/review.
Yeah, that's why I opted to just hardcode it directly in the action. Also, all callers either passed "stable" already or forgot to set it entirely.
Building on top of #80 and combining this with qemu-binfmt we can now run ARM64 tests in CI despite the continuing lack of ARM64 runners on GitHub (Microsoft, please fix!).