-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use shellcheck #10
Comments
Thanks for raising this It is used in CI/CD, but yes pre-commit has been on the backlog Unfortunately it doesn't catch that typo in the shebang line: $ shellcheck gcp/gcp_service_accounts_credential_keys_expired.sh
$ echo $?
0 I've just fixed that shebang line. |
@HariSekhon oh dang, you're right! i found that when looking at which shebang lines were used in this repo, not through shellcheck.
|
Thanks for finding this one though, that's fixed and I'll keep pre-commit on the todo list anyway as I had other plans for it too |
Shellcheck (https://www.shellcheck.net) exists to find syntax errors in shell code, including bash and posix sh. It finds about 190 syntax problems within this repo, including 4 errors and 62 warnings. I would recommend using the a pre-commit shellcheck hook (https://github.com/koalaman/shellcheck-precommit), or run shellcheck as a CI action (https://github.com/marketplace/actions/shellcheck), in order to avoid giving users confidence that the scripts here will behave a certain way, but then fail because of some syntax nuance or outright syntax error. It looks like shellcheck has been used in this heavily before because there are over 3k instances of shellcheck in the code, but is not being enforced everywhere.
For instance, https://github.com/HariSekhon/DevOps-Bash-tools/blob/262f684d1e/gcp/gcp_service_accounts_credential_keys_expired.sh#L1 has existed in the repo for 3 years, but is almost certainly a human introduced typo which is an obvious syntax error, and really easy to fix once spotted.
The text was updated successfully, but these errors were encountered: