Skip to content
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

Add shellcheck dependency in pre-commit hooks #482

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kadykov
Copy link

@kadykov kadykov commented Nov 21, 2024

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.

@rhysd
Copy link
Owner

rhysd commented Nov 24, 2024

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.

@anuraaga
Copy link
Contributor

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.

@rhysd
Copy link
Owner

rhysd commented Dec 3, 2024

@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.

@rdctmeconomou
Copy link

@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.

@AlexWaygood
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pre-commit hook does not install shellcheck, resulting in inconsistent linter behavior
5 participants