Skip to content

[actions] Add shellcheck workflow#2351

Merged
ljharb merged 1 commit intonvm-sh:masterfrom
reasonablytall:alexaub/shellcheck-workflow
Nov 19, 2020
Merged

[actions] Add shellcheck workflow#2351
ljharb merged 1 commit intonvm-sh:masterfrom
reasonablytall:alexaub/shellcheck-workflow

Conversation

@reasonablytall
Copy link
Contributor

This adds a github workflow that runs shellcheck on nvm.sh and install.sh. nvm.sh is tested with each shell in [bash, sh, dash, ksh] and install.sh is tested with just bash. I think submitting this PR should run the workflow, but just in case, here is an example of this change running on my fork.

Unfortunately shellcheck does not support zsh, which is why it's omitted from the matrix.

I ignored SC1001 in nvm.sh because there are three instances where \command ... is used (i'm guessing) to suppress aliases. More recent versions of shellcheck automatically ignore these instances, but not the older version available on the ubuntu-based github runners. Alternatively, I could ignore the specific lines in nvm.sh rather than globally ignoring SC1001, or I could figure out a way to manually install a more recent version of shellcheck. Let me know!

Co-authored-by: Alex Aubuchon <alex@aub.dev>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@reasonablytall
Copy link
Contributor Author

Hmm looking at the list of checks created the naming looks a little messy, let me know if you have any preferences with those!

Also, maybe I should make checking install.sh a separate job, rather than a subtle part of the bash run?

@ljharb
Copy link
Member

ljharb commented Nov 19, 2020

Yes, it should be a separate job in the matrix.

It'd be ideal to find a way to get the latest shellcheck; i rely on the things the latest version checks.

@ljharb ljharb force-pushed the alexaub/shellcheck-workflow branch 4 times, most recently from acf28e4 to f7f0e37 Compare November 19, 2020 20:12
@ljharb
Copy link
Member

ljharb commented Nov 19, 2020

k, fixed the matrix, the only thing is that this is v0.4.6, and shellcheck's up to v0.7.1. is brew available in actions? if so, brew install shellcheck would work; otherwise maybe we need to use a custom apt repo?

@reasonablytall reasonablytall force-pushed the alexaub/shellcheck-workflow branch 2 times, most recently from 12c1606 to 57ffe2a Compare November 19, 2020 21:42
@reasonablytall
Copy link
Contributor Author

Brew does work! Thanks for the matrix changes too; I wouldn't have thought of doing it that way :)

I removed the ignore SC1001 declaration as well, now that we have the most recent version of shellcheck.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

woot, looks great!

@ljharb ljharb force-pushed the alexaub/shellcheck-workflow branch from 57ffe2a to 3abb981 Compare November 19, 2020 21:58
@ljharb ljharb merged commit 3abb981 into nvm-sh:master Nov 19, 2020
@reasonablytall reasonablytall deleted the alexaub/shellcheck-workflow branch November 19, 2020 22:29
@reasonablytall reasonablytall restored the alexaub/shellcheck-workflow branch November 19, 2020 22:29
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