-
Notifications
You must be signed in to change notification settings - Fork 111
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 a bash linter to pre-submits #781
base: master
Are you sure you want to change the base?
Conversation
Fixes the following shellcheck error: SC2068 (error): Double quote array expansions to avoid re-splitting elements. https://www.shellcheck.net/wiki/SC2068
Fixes the following shellcheck error: SC2148 (error): Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive. https://www.shellcheck.net/wiki/SC2148
Fixes the following shellcheck errors: SC2145 (error): Argument mixes string and array. Use * or separate argument. SC2199 (error): Arrays implicitly concatenate in [[ ]]. Use a loop (or explicit * instead of @). https://www.shellcheck.net/wiki/SC2145 https://www.shellcheck.net/wiki/SC2199 Also fixes a typo in SUPPORTED_INTERFACE_SWITCHER_MODES.
Fixes the following shellcheck error: SC2045 (error): Iterating over ls output is fragile. Use globs. https://www.shellcheck.net/wiki/SC2045
Thanks for your PR,
To skip the vendors CIs, Maintainers can use one of:
|
Pull Request Test Coverage Report for Build 11066086546Details
💛 - Coveralls |
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.
Great work!
Thanks for the contribution!
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.
just a nit: can you put it inside the same test.yml file and not create a new one please
a18bf43
to
452f4e7
Compare
test/scripts/rpm-ostree_mock
Outdated
@@ -5,8 +5,8 @@ | |||
# Write invocation with arguments to a file to allow making assertion. | |||
echo "$*" >> /rpm-ostree_calls | |||
|
|||
if [ "$*" != *"--append"* ] | |||
then | |||
# if [ "$*" != *"--append"* ] |
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.
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.
@zeeke can you have a look at this please
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.
With this change, the test fail with:
+ chroot /host/ rpm-ostree kargs
+ /usr/sbin/chroot /tmp/tmp.gOQJTP0gp3 rpm-ostree kargs
+ grep -vq X=Y
/usr/sbin/chroot: failed to run command ‘rpm-ostree’: No such file or directory
Which doesn't really make sense to me. I'll try changing the shebang back to #!/usr/bin/sh
to see if it makes a difference.
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.
OK, a5c53b5 worked.
I think this is ready for reviews now.
Fixes the following shellcheck error: SC2081 (error): [ .. ] can't match globs. Use a case statement. https://www.shellcheck.net/wiki/SC2081
Warns about shellcheck issues with severity `error`.
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.
nice work!
LGTM
Add a new CI job that warns about shellcheck issues with severity
error
, and fix all such errors in existing bash files.This should prevent errors such as the one fixed in #780 from entering the repo again.