ci: use official action to setup rust toolchain #585
Conversation
roeap
force-pushed
the
ci/toolchain-action
branch
from
b779455 to
f334d14
Compare
kernel/src/engine/arrow_data.rs
Outdated
There was a problem hiding this comment.
This was my auto-formatter in the IDE, since there are cargo fmt checks in ci, nut sure why that happened. In most cases I do like the prior formatting better though, do we a have some line-length config somewhere I am not picking up?
There was a problem hiding this comment.
hm that's interesting - guessing your editor is applying something 'extra' on top of rustfmt? I do prefer the old formatting, can we revert these changes?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 83.44% 83.47% +0.02%
==========================================
Files 74 74
Lines 16861 16849 -12
Branches 16861 16849 -12
==========================================
- Hits 14069 14064 -5
+ Misses 2132 2128 -4
+ Partials 660 657 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
nice thanks! changes to existing CI LGTM, just a question on adding dependabot and also overall question: it looks like the two most popular rust toolchains in CI are dtolnay/rust-toolchain and actions-rust-lang/setup-rust-toolchain with the latter saying that it was heavily inspired by dtolnay's? I guess this has caching built-in as the main benefit?
kernel/src/engine/arrow_data.rs
Outdated
There was a problem hiding this comment.
hm that's interesting - guessing your editor is applying something 'extra' on top of rustfmt? I do prefer the old formatting, can we revert these changes?
.github/dependabot.yaml
Outdated
There was a problem hiding this comment.
oh nice! are we planning on enabling dependabot for the repo?
There was a problem hiding this comment.
well - i guess thats up for debate. but since this was literally just copy and paste from somewhere else, it was low cost to spark the discussion by just providing the solution 😆.
From my end, I think the utility may unfortunately be limited as auto-updates for the major dependencies (e.g. arrow) almost never work out of the box - at least in delta-rs. That said, I still landed on "let's add it" to keep the actions fresh and have the occasional rust version bump that works.
There was a problem hiding this comment.
yea I think it is helpful insofar as it just automatically opens small PRs to bump versions and we may (probably) have to manually help upgrade but the 'reminder' to upgrade seems useful? curious to hear what others think
There was a problem hiding this comment.
the dependabot-quickstart-guide doesn't recommend setting up manually like this, but rather suggests going into settings and letting github create this file. Wondering if we should just do it that way?
There was a problem hiding this comment.
I'm okay trying this out, but also a bit skeptical of the utility here. As far as I can tell it only runs cargo update. Generally with how we've set up our Cargo.toml this will only do minor updates (i.e. it'll never update arrow 53->54). Maybe that's good, but we need a process to also run cargo outdated and try and stay on top of more major updates as well, so I'm not sure how much time this saves us.
|
just want to make sure: is the caching working? I see the following errors in test (ubuntu-latest) in the post-run step: |
| @@ -109,7 +92,7 @@ jobs: | |||
| - name: Setup cmake | |||
| uses: jwlawson/actions-setup-cmake@v2 | |||
| with: | |||
| cmake-version: '3.30.x' | |||
| cmake-version: "3.30.x" | |||
There was a problem hiding this comment.
In yaml double quotes allow escapes and other things, so I tend to prefer single quotes unless I need that, as, fewer surprises. Any reason you changed this?
There was a problem hiding this comment.
auto-formatting ... maybe i need to make prettier or whoever it was less agressive :).
.github/dependabot.yaml
Outdated
| @@ -0,0 +1,14 @@ | |||
| # Please see the documentation for all configuration options: | |||
| # https://help.github.com/github/administering-a-repository/configuration-options-for-dependency-updates | |||
There was a problem hiding this comment.
This link doesn't seem to work. it just goes to a generic support page
.github/dependabot.yaml
Outdated
There was a problem hiding this comment.
the dependabot-quickstart-guide doesn't recommend setting up manually like this, but rather suggests going into settings and letting github create this file. Wondering if we should just do it that way?
.github/dependabot.yaml
Outdated
There was a problem hiding this comment.
I'm okay trying this out, but also a bit skeptical of the utility here. As far as I can tell it only runs cargo update. Generally with how we've set up our Cargo.toml this will only do minor updates (i.e. it'll never update arrow 53->54). Maybe that's good, but we need a process to also run cargo outdated and try and stay on top of more major updates as well, so I'm not sure how much time this saves us.
What changes are proposed in this pull request?
The
actions-rs/toolchainaction is deprecated in favor ofactions-rust-lang/setup-rust-toolchain. This PR updates the usages of the respective actions in the github workflows. THe new action already includes an integration with the rust-cache action, so no need to set that up separately anymore.This also sets up a dependabot configuration for
cargoandgithub-actionswhich we may or may not choose to keep.How was this change tested?
no code changes.