Setup shellcheck and fix all issues #189
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
shellcheck
🚫 [shellcheck] reported by reviewdog 🐶
SIGKILL/SIGSTOP can not be trapped. SC2173
Line 238 in ff8a494
[shellcheck] reported by reviewdog 🐶
expr is antiquated. Consider rewriting this using
Line 278 in ff8a494
Declare and assign separately to avoid masking return values. SC2155
Line 281 in ff8a494
[shellcheck] reported by reviewdog 🐶
expr is antiquated. Consider rewriting this using
Line 281 in ff8a494
Declare and assign separately to avoid masking return values. SC2155
Line 285 in ff8a494
[shellcheck] reported by reviewdog 🐶
expr is antiquated. Consider rewriting this using
Line 285 in ff8a494
[shellcheck] reported by reviewdog 🐶
Line 415 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Use '[:lower:]' to support accents and foreign alphabets. SC2018
Line 451 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Use '[:upper:]' to support accents and foreign alphabets. SC2019
Line 451 in ff8a494
Declare and assign separately to avoid masking return values. SC2155
Line 495 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
read without -r will mangle backslashes. SC2162
Line 558 in ff8a494
Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. SC2166
Line 559 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 571 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 572 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 573 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 574 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 575 in ff8a494
Declare and assign separately to avoid masking return values. SC2155
Line 644 in ff8a494
Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames. SC2010
Line 656 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Use '[:lower:]' to support accents and foreign alphabets. SC2018
Line 687 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Use '[:upper:]' to support accents and foreign alphabets. SC2019
Line 687 in ff8a494
Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. SC2166
Line 754 in ff8a494
Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. SC2166
Line 760 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 769 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 769 in ff8a494
Expanding an array without an index only gives the first element. SC2128
Line 788 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
To read lines rather than words, pipe/redirect to a 'while read' loop. SC2013
Line 898 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 918 in ff8a494
Declare and assign separately to avoid masking return values. SC2155
Line 995 in ff8a494
Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames. SC2010
Line 1081 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Use find instead of ls to better handle non-alphanumeric filenames. SC2012
Line 1091 in ff8a494
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1111 in ff8a494
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1130 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1136 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1144 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1152 in ff8a494
For loops over find output are fragile. Use find -exec or a while read loop. SC2044
Line 1349 in ff8a494
[shellcheck] reported by reviewdog 🐶
Consider using { cmd1; cmd2; } >> file instead of individual redirects. SC2129
Line 1377 in ff8a494
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1409 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1463 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1463 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1463 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1497 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Make sure not to read and write the same file in the same pipeline. SC2094
Line 1536 in ff8a494
📝 [shellcheck] reported by reviewdog 🐶
Make sure not to read and write the same file in the same pipeline. SC2094
Line 1536 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in ff8a494
Use 'cd ... || exit' or 'cd ... || return' in case cd fails. SC2164
Line 1691 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in ff8a494
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in ff8a494
Use 'cd ... || exit' or 'cd ... || return' in case cd fails. SC2164
Line 1811 in ff8a494
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
shellcheck
Don't use ls | grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames. SC2010
Line 656 in 9e89e1a
Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. SC2166
Line 754 in 9e89e1a
Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. SC2166
Line 760 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 769 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 769 in 9e89e1a
📝 [shellcheck] reported by reviewdog 🐶
To read lines rather than words, pipe/redirect to a 'while read' loop. SC2013
Line 898 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 918 in 9e89e1a
Declare and assign separately to avoid masking return values. SC2155
Line 995 in 9e89e1a
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1111 in 9e89e1a
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1130 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1136 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1144 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1152 in 9e89e1a
[shellcheck] reported by reviewdog 🐶
Consider using { cmd1; cmd2; } >> file instead of individual redirects. SC2129
Line 1377 in 9e89e1a
This $? refers to echo/printf, not a previous command. Assign to variable to avoid it being overwritten. SC2320
Line 1409 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1497 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1607 in 9e89e1a
Use 'cd ... || exit' or 'cd ... || return' in case cd fails. SC2164
Line 1691 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in 9e89e1a
Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. SC2166
Line 1709 in 9e89e1a
zeha
force-pushed
the
shellcheck
branch
5 times, most recently
from
596da04 to
fe3eef9
Compare
|
Running a local test build right now. I hope I didn't break anything, but not super confident. |
|
FWIW: We've been using https://github.com/redhat-plumbers-in-action/differential-shellcheck which has the benefit of only failing on newly added violations, not on existing offenses |
Ah, very cool. I've "just" fixed all issues ;-) |
|
Yeah, I'm never that motivated when working with shell ;) |
This found an issue or two which I've fixed. |
scripts/generate_netboot_package.sh
Outdated
| @@ -43,7 +43,7 @@ cat > "${WORKING_DIR}/pxelinux.cfg/default" << EOF | |||
| default grml | |||
| label grml | |||
| kernel vmlinuz | |||
| append initrd=initrd.img root=/dev/nfs rw nfsroot=192.168.0.1:/live/image boot=live apm=power-off quiet nomce noprompt noeject vga=791 net.ifnames=0 | |||
| append initrd=initrd.img root=/dev/nfs rw nfsroot=192.168.0.1:/live/image boot=live apm=power-off quiet nomce noprompt noeject vga=791 net.ifnames=0 | |||
There was a problem hiding this comment.
please don't drop the trailing whitespace char here (it's relevant when e.g. someone wants to append something like ssh=foobar)
There was a problem hiding this comment.
Ack, that was an overzealous text editor and an oversight on my part.
| @@ -23,17 +26,17 @@ if [ "$FAI_ACTION" = "softupdate" ] ; then | |||
|
|
|||
| # /etc/resolv.conf is usually a symlink, pointing out of the chroot. | |||
| # Make it a file with known contents. | |||
| rm -f "${target}"/etc/resolv.conf | |||
| rm -f "$target"/etc/resolv.conf | |||
There was a problem hiding this comment.
nit: we have a strange mixture of "${target}" vs "$target" now :p
(I'm aware that this is inconsistent over all the code base, nowadays I prefer to use "$f" and "${foo}" (so no ${} for single char variables, whereas ${} for >=2 chars)
There was a problem hiding this comment.
Yeah, I tried to aim for some local consistency... at some point I gave up. We need to make a call and then update all places in one go.
There was a problem hiding this comment.
Fixed at least instsoft.GRMLBASE to be in one style overall.
| @@ -29,9 +32,10 @@ echo "$0: Installing latest kernel and its headers, as well as build-essential." | |||
| # keeping track of what gets installed. This is an ugly hack and should not | |||
| # be needed, but without it the resulting ISO is hundreds of megabytes | |||
| # larger. I hope this kludge can go away eventually. | |||
| extra_packages=($($ROOTCMD apt-get --assume-no --download-only --mark-auto -u install \ | |||
| mapfile -t extra_packages <($ROOTCMD \ | |||
There was a problem hiding this comment.
this doesn't work, AFAICT, needs to be:
mapfile -t extra_packages < <($ROOTCMD ...
There was a problem hiding this comment.
Indeed. It also needs some extra sed to actually work. Fixed both instances.
| @@ -54,7 +58,11 @@ else | |||
| fi | |||
|
|
|||
| echo "$0: Installing zfs-dkms itself." | |||
| extra_packages=(${extra_packages[@]} $($ROOTCMD apt-get --assume-no --download-only --mark-auto -u install zfs-dkms | sed '0,/The following NEW packages will be installed/d;/^[^ ]/,$d')) | |||
| mapfile -t zfs_packages <($ROOTCMD \ | |||
Used by init-functions. Until we replace that, add this annotation.
AFAICT nothing uses SHORT_NAME outside of grml-live itself.
Declare and assign separately to avoid masking return values.
read without -r will mangle backslashes. Not relevant here.
$/${} is unnecessary on arithmetic variables.
Setup shellcheck in GitHub Actions and fix all reported issues.