-
Notifications
You must be signed in to change notification settings - Fork 41
Allow the compute-sdk tests to run without building locally #129
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
Conversation
e72fc3c to
b1b98e0
Compare
8e6c29e to
b7e2fd9
Compare
b7e2fd9 to
e6fbc1e
Compare
|
I haven't really reviewed this yet, but a quick thought regarding the install-rust action specifically: it's almost empty now, it should probably be a shell fragment in a composite action instead, or even just be inlined into the only place it's used in the main workflow. |
Seems reasonable to me! I switched it over to a composite action in the third commit. |
7e1f7a0 to
4818fb7
Compare
| echo 'CARGO_PROFILE_DEV_DEBUG=1' >> "$GITHUB_ENV" | ||
| echo 'CARGO_PROFILE_TEST_DEBUG=1' >> "$GITHUB_ENV" | ||
| - if: ${{ ! startsWith(inputs.toolchain, 'nightly') }} |
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.
Oh yeah, this reminds me: I broke the "toolchain" input when I switched this over to just reading rust-toolchain.toml. That input no longer controls the toolchain that's getting installed. So this test doesn't do what any reasonable person would think it's doing. I definitely agree this commit accurately translates main.js as it stands now into a composite action though!
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.
We should figure out if we'd like to keep this. It seems nice to have a reusable place to setup the environment variables we're relying on, but the action name seems a bit misleading now :)
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.
rustup show does install the toolchain though, as a side-effect. 😆 Maybe I should have commented to that effect when I made that change... It just has the nice bonus of recording in the CI log exactly what toolchain it ran with.
This change allows us to run
node .github/actions/compute-sdk-test/main.jswithout first runningnpm buildin that directory. We still need to runnpm installthere, but this simplifies iteration when working locally.I also updated the
install-rustaction tonode16while I was at it.