-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Introduce shellcheck to lint shell scripts #15169
base: master
Are you sure you want to change the base?
Changes from 3 commits
076421e
696b97f
b5970e8
614183b
cb9047b
90a487f
53ede77
6928049
639c57b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
on: | ||
push: | ||
pull_request: | ||
|
||
name: "Format checks" | ||
permissions: {} | ||
|
||
jobs: | ||
shellcheck: | ||
name: Shellcheck | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Run ShellCheck | ||
uses: ludeeus/action-shellcheck@master | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super happy with running an arbitrarily changing action with an arbitrarily changing version of shellcheck. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be done in a follow-up, I suppose 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. I just picked this up as it's an easy solution. I'm happy to use any other mechanism. We could also simply use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Integration with pre-commit would also be nice 🤷 |
||
with: | ||
# shellcheck doesn't have good support for zsh, so we're ignoring this | ||
ignore_paths: >- | ||
*.zsh | ||
severity: error | ||
|
||
- name: Run ShellCheck Warnings | ||
uses: ludeeus/action-shellcheck@master | ||
with: | ||
ignore_paths: >- | ||
./etc/completion.zsh | ||
./etc/completion.bash | ||
./etc/win-ci/cygwin-build-iconv.sh | ||
severity: warning |
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.
Might be worth defining
paths
here. Supposedly this check could then run very rarely, only when neededhttps://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore
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.
Yeah, I was wondering about that as well. A bit of an obstacle is that there's no simple glob to catch all shell script. Some like
bin/crystal
have no extension. The GitHub action actually checks all files for a matching shebang.Limiting paths would need to hard code all paths that don't identify as shell script by their extension. That means the check could miss out on new ones being added.
I suppose this is an acceptable limitation, though. I wouldn't expect too much movement in that regard.
On the other hand, this job finishes reallly quickly - especially compared to our other workflows - in just 7s so it's not too much overhead to run it on every commit.
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.
Yeah it's a fair point. Not a good tradeoff then
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'm actually not so sure. Either way seems quite fine. So maybe we can hear some other's comments on this.
It's also relevant to note that the discovery feature depends on the GitHub action and another comment is suggesting we might do without the action. Droping automatic path discovery would make that easier.