-
Notifications
You must be signed in to change notification settings - Fork 31
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
ci: add shell linting workflow #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this affects more than the packages I am responsible off, I think at least someone else should be aware and approve this change?
I think it is quite good
Yeah, this will cause a considerable amount of warnings/errors the next time someone touches a shell script. I can go ahead and fix (most of) them if needed beforehand. |
Is there a link where one can see which scripts (in which way) this will affect? I've got no idea wheter shellcheck is a fitting choice for this repo. And yes, i think fixing those scripts should be done beforehand and not by the next person who finds themself in the unfortunate situation to trigger that workflow ;) |
Shellcheck helped me identify a range of bugs in the ssid-changer script, so I do think that it is a helpful addition in general. You can run this command locally, that roughly does what the action does:
|
I did run:
to auto fix some issues - but most have to be solved manually. There are still a lot here: and the like.. I don't think that it is beneficial to have a lot of shellcheck workarounds though :D |
Added exemplary PRs that fix the shellcheck issues in the ffac-ssid-changer package: |
Regarding
X=1
[ $X = 1 ]
[ $X == 1 ]
ash: 3: [: 1: unexpected operator
X=1
[ $X = 1 ]
[ $X == 1 ]
dash: 3: [: 1: unexpected operator
X=1
[ $X = 1 ]
[ $X == 1 ] Shellcheck has support for full bash, ksh and dash, but not the full set for |
I opened an issue with shellcheck to see if it can be improved for our use-case here: |
I opened PRs for all outstanding shellcheck issue and assigned it to the "suggested" reviewer from Github:
These PRs should address all shellcheck issues. |
@maurerle @herbetom @neocturne except the #61, all shellcheck issues should be resolved now and the CI passes at least on my local machine. |
A proposal to add some shell linting as there are quite some shell scripts in this repository.