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

Use shellcheck #10

Open
danielhoherd opened this issue Nov 20, 2023 · 3 comments
Open

Use shellcheck #10

danielhoherd opened this issue Nov 20, 2023 · 3 comments

Comments

@danielhoherd
Copy link

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.

@HariSekhon
Copy link
Owner

HariSekhon commented Nov 20, 2023

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.

@danielhoherd
Copy link
Author

@HariSekhon oh dang, you're right! i found that when looking at which shebang lines were used in this repo, not through shellcheck.

$ git grep -h '^#!' | sort | uniq -c
   2 #!**/[Pp]ackages/repositories.config
   1 #!/bin/bash
  23 #!/bin/sh
   1 #!/u)sr/bin/env bash
1151 #!/usr/bin/env bash
   9 #!/usr/bin/env osascript
   1 #!/usr/bin/env python3
   1 #!/usr/local/bin/expect -f

@HariSekhon
Copy link
Owner

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

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

No branches or pull requests

2 participants