-
Notifications
You must be signed in to change notification settings - Fork 29
Make linter action more ergonomic #501
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
f2721eb
to
fb9e9b2
Compare
8a74023
to
342f1fe
Compare
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.
I cant test right now, but the code makes sense!
If it works in a separate repository feel free to merge this :)
--path "${BEVY_LINT_PATH}" \ | ||
--locked | ||
env: | ||
RUST_CHANNEL: ${{ steps.toolchain.outputs.channel }} |
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: We could remove the env variable here and directly use the step output, right?
No strong preference 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.
I'd prefer not to. While it's not an issue in this specific case, directly inlining templates can lead to code injection vulnerabilities, so I avoid doing so out of habit! (It's the same reason I use "${VAR}"
:)
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.
Ok, that's fine for me :)
@@ -12,20 +12,39 @@ description: | | |||
runs: | |||
using: composite | |||
steps: | |||
# Used to read `rust-toolchain.toml`. | |||
- name: Install Taplo | |||
uses: taiki-e/install-action@v2 |
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.
Since we control the toolchain file and it doesn't look too complex, I wonder if we could accomplish the same without installing an additional tool.
That could speed up the CI time.
On the other hand, we'd need to be careful to make it work on all platforms, so it probably makes sense to investigate this in a separate issue.
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.
I think this would be a good follow-up. We could probably use grep
to capture the toolchain version with a few clever comments, like what Clippy does.
Tested in |
Context
The Github Action we provide for the linter isn't great. It works well for installing from a release or from
main
, but breaks down when installing a specific commit. This is because we currently hardcode values inaction.yml
:bevy_cli/bevy_lint/action.yml
Lines 18 to 19 in ff8d39a
bevy_cli/bevy_lint/action.yml
Lines 25 to 31 in ff8d39a
This is an extra maintenance burden that we shouldn't have to do! If the user wants to install a specific commit of the linter, it should work as expected instead of installing the
main
branch:Solution
Thankfully, the fix is easier than expected.
action.yml
is a composite action, meaning it's a single YAML file that slings around other actions and shell scripts to do things. The great thing about this is that, when an action runner downloads a composite action, it downloads the entire repository. That meansrust-toolchain.toml
and the correct source code forbevy_lint
already exist, we just have to install them!This PR does just that. It uses the venerable Taplo to read
rust-toolchain.toml
and then runscargo install --path ...
to install the linter. The action especially uses the${{ github.action_path }}
variable to locate thebevy_lint
folder.Testing
linter-action.yml
is doing a lot of the heavy-lifting here by testing the linter action in CI. I'll also spin up a separate repository to prove it works for others as well!