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 Lint, Address Caught Issues #849

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

Conversation

codecriticsceo
Copy link

PR adds the shellcheck linter to check the shell scripts, and fixes the found issues. Nothing major, although the existing scripts might not work if there are spaces or asterisks in the development repo path. If you would like changes, please let me know.

codecriticsceo added 2 commits December 28, 2024 21:42
Signed-off-by: codecriticsceo <codecriticsceo>
Signed-off-by: codecriticsceo <codecriticsceo>
@codecriticsceo codecriticsceo changed the title Add Shellcheck Lint, Caught Issues Add Shellcheck Lint, Address Caught Issues Dec 29, 2024
Comment on lines 20 to 21
- name: Run Shellcheck
run: find . -name '*.sh' -print0 | xargs -0 shellcheck -x
Copy link
Member

@broofa broofa Dec 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use the shellcheck GH action, instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did not use it was because I don't think that's an official Shellcheck project action. Here is the page on shellcheck: https://www.shellcheck.net/wiki/GitHub-Actions. Would be happy to switch, if you want though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. That seems simple enough. Maybe just change this to the recommended incantation...

    - run: shellcheck **/*.sh

I don't think the find command is needed. Nor the -x option (what does that even do?)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -x option follows included shellscripts and checks them as well. The find command is so that shellscripts in varying directory levels (including root) will all be found.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can test with the action, but in bash, echo **/*.sh appears not to find shellscripts in the immediate directory, only a subdirectory

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update tested: https://github.com/codecriticsceo/uuid/actions/runs/12554064345/job/35002278202. I added two bad scripts to a throw-away branch, one in root and one two directories down. shellcheck **/*.sh did not find them but the find command did. I am going to try updating the shellcheck wiki.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR with the exact command from the docs (find without -x or xargs)

@broofa
Copy link
Member

broofa commented Dec 30, 2024

Noting for posterity that shellcheck -f diff **/*.sh | git apply can be used to auto-fix shellcheck issues.

Signed-off-by: codecriticsceo <codecriticsceo>
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.

2 participants