chore(#8557): add linting for shell scripts #9142
Conversation
nydr
changed the title
nydr
changed the title
.github/workflows/test_shell.yml
Outdated
| pull_request: | ||
| paths: | ||
| - '**/*.bats' | ||
| - '**/*.sh' |
There was a problem hiding this comment.
Not convinced conditional run is the right thing to do here as it only takes 30 sec including install step, but I'm hesitant to add another dependency on the main test since it's already fairly cluttered.
There was a problem hiding this comment.
I think it should be part of the npm run lint command so devs just run one command and everything is linted, but it would be a shame to add more to the dev environment setup requirements. There is an npm package that might be useful to do the installation for us. Might be worth looking in to...
There was a problem hiding this comment.
That's a good suggestion, I'm also making use of shfmt to list all shell scripts which is needed for shellcheck to find and test those files. I can't use *.sh and *.bats since not all of them have those extensions, but something like
for f in $(git ls-files); do
head -n1 $f | egrep -q '#!/bin/(ba)?sh' && echo $f;
donemight work as an alternative
(also need to consider scripts with /usr/bin/env bash and *.bats extensions so not exactly this script)
garethbowen
changed the title
|
Changed the PR to "chore" not "feat" as it's not a feature from an end user perspective. |
|
PR updated with adding shellcheck npm package dependency as suggested and replacing the usage of Feel free to take over this PR if anyone has the bandwith as I'm unlikely be able to update in the foreseeable future. Keep in mind that it is technically changing the behaviour of ~22 scripts with low to no related tests even if most of these behaviour changes are unlikely to be hit and if they were the previous behaviour is most likely the undesired one |
There was a problem hiding this comment.
Thank you! This has been bugging me for some time.
I agree there are risks with existing scripts but we probably won't find any bugs until we merge so I vote we go for it.
@mrjones-plip I'll wait for you to you review too to make sure you concur!
|
While I do concur - and think this is worthwhile effort - we're touching a LOT of scripts. I think we should smoke test each script that got updated - skipping any that will hit CI right away that might be hard to trigger locally. I'll grab this next week! |
…y unsafe with shellcheck disable=SC2086
Description
Adds linting to shell script and update scripts to pass, there's some cases where this could change the functionality so pay extra attention to the sections of the script relying on unexpected behavior
per #8557
Affected bash scripts - please check off when you have confirmed they work with the additions in this PR:
#shellcheckcomment#shellcheckcomment#shellcheckcommentCode review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.