-
Notifications
You must be signed in to change notification settings - Fork 39
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
chore(IDX): move some tests to self-hosted runners #5258
Conversation
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.
My understanding of this is low (therefore, no approve). However, FWIW, AFAICT, LGTM.
AFAICT, the main thing this does is change where things get run, but the new place has a very similar platform, resulting in almost identical behavior/results.
The other substantive thing I see going on here is
rustup default stable
Not sure why this is needed. I guess changing where things run implicitly changes the default toolchain away from stable (one of the small platform differences). Therefore, I guess this is needed to preserve stable, yes?
Hey thanks for your input. I don't use Rust, so I am not familiar with the best setup. I added this step because I got the following error (workflow run)
I'm open to any other suggestions! |
I'm not too familiar with Rust either but it was my understanding that https://github.com/dfinity/nns-dapp/blob/main/rust-toolchain.toml defines the version to use. |
We can - do you have an example from another part of the codebase for how it consumes the correct version?
You can see here the rust version that was pre-installed on |
I'm not sure I understand the question but that file is regularly updated to increase the version and sometimes that results in new lint errors requiring changes. So changing that version definitely has some effect. See for example #5243 Also, if I run
Yes, we should be using the Rust version specified in |
I think I found the issue - cargo needed to be run from inside the working directory to use the version in
Based on my understanding, it looks like you were using the cargo version specified in |
I don't think we were being lucky, because as I said, PRs that changed the version in But on line 50 of Also since we're not actually building our wasm (only spellcheck), we don't really care what version of Rust we're using. Isn't it reasonable to have some default specified on the custom runner, similar to GitHub runners? I think in general for easier migration you might want to have your custom runners as similar as possible to GitHub runners. |
Ultimately your team owns the repo and the CI, so you can decide the best implementation. It looks like you were maybe using
Currently no, because different repos could require different versions and we're not in a spot where we can maintain different images with different versions yet. |
I prepared a PR to remove the |
I don't follow this. GitHub runners also don't maintain different images for different versions of Cargo. I'm not saying we should select the Cargo version via the image. I'm just saying that it might be easier to migrate (other jobs and other repos) if DFINITY runners are more similar to GitHub runners. |
Motivation
Now that we have moved the
ic
repo to github, we need to be more careful about using github-hosted runners so we don't run out of minutes for critical workflows. This PR moves several jobs that have shown up as consuming high volumes of minutes to self-hosted runners.Changes
Use self-hosted runners with a container image we maintain. No code changes required.
Tests
All tests passed.
Todos