Add `shellcheck` dependency in pre-commit hooks
Fixes #477
This pull request adds the github.com/wasilibs/go-shellcheck/cmd/shellcheck@latest package as a dependency for actionlint pre-commit hooks. This change resolves the issue reported in #477.
Please note that I'm not familiar with Go and may not have chosen the most optimal dependency package. If you have a better approach, please let me know and I'll be happy to update the code.
Since I'm not a pre-commit user, I can't say this is the right direction or not. However let me confirm one thing. Why do you use the WASI port of shellcheck instead of the official released binaries?
The go-shellcheck project looks very young. It's unclear how it will be maintained (e.g. following the new releases). I'm not sure that we should rely on the project.
It's unclear how it will be maintained (e.g. following the new releases)
Randomly noticed this PR and just wanted to answer this point, there is an automatic update script for catching new upstream releases
https://github.com/wasilibs/go-shellcheck/blob/main/.github/workflows/update.yaml
shellcheck is the latest project but there are several other ones in wasilibs and it's been pretty easy to keep up with releases thanks to the automated updates.
That being said, I agree with the sentiment of sticking to official repos so if there's an easy way to use the official binaries with pre-commit, it's probably better, just wanted to clarify the above point.
@anuraaga Thank you for the explanation. Then I'm okay to rely on go-shellcheck.
However I'm not a pre-commit user as I said. So I'd like to hear opinion from some practical pre-commit users.
@anuraaga hey, thanks a ton for this! I opened the original issue, so I feel obligated to give this a spin. @rhysd I should have time toward the end of next week, if that's OK with you.
I'm a pre-commit user, and I can confirm:
- That using actionlint via pre-commit by default does not enable the shellcheck integration
- That the changes made in this PR look like they would enable the shellcheck integration by default when running actionlint via pre-commit
However, I'm not sure this is the best approach. Using additional_dependencies: ["github.com/wasilibs/go-shellcheck/cmd/shellcheck@latest"] means that pre-commit will always install the latest version of shellcheck, which could lead to your CI unexpectedly and unpredictably breaking if shellcheck cuts a new release and you're running actionlint in CI via pre-commit. Another approach might be to add a note to actionlint's documentation saying that users will need to explicitly list shellcheck as an additional dependency when they use actionlint as a pre-commit hook if they want the shellcheck integration. That way users are forced to consider which shellcheck version they want installed by pre-commit as part of the actionlint hook, and are able to easily pin shellcheck to a specific version.
You can take a look at the pre-commit configuration I'm adding in https://github.com/astral-sh/ruff/pull/15021/files as an example of how to use actionlint as a pre-commit hook while enabling the shellcheck integration
Thank you for the discussion.
Then I feel the followings are right directions:
- Create a new pre-commit hook which utilizes
go-shellcheckin addition toactionlint,actionlint-docker,actionlint-systemfor those who prefer the go-shellcheck approach. - Update the configuration example in the pre-commit guide to setup both actionlint and shellcheck as @AlexWaygood pointed.