Fix 'shellcheck' complaints#193
Fix 'shellcheck' complaints#193sinnykumari merged 2 commits intocoreos:masterfrom miabbott:shellcheck_fixes
Conversation
|
Inspired by Luca's comment, I took a stab at correcting most of the But I haven't actually verified everything works yet, so WIP |
|
Seems sane, I didn't see any problems offhand just skimming it. But today we don't have any CI so...please do test it. I'd probably say we should aim to land this after #190 since this is a nice-to-have but that's more critpath. |
Yeah, I'd like to land this eventually as a start for some basic CI syntax checking/linting. |
sinnykumari
left a comment
sinnykumari
left a comment
There was a problem hiding this comment.
LGTM
Nice work @miabbott !
|
thanks @cgwalters and @sinnykumari for the review. I will merge this after #190 is merged and this is rebased. |
|
@dustymabe I'm still making more changes...forgot to check some of the other bash scripts...I'm in deep now. 😆 Let's wait until after #190 and I'll rebase and re-run |
|
This should be ready for review. Most of the changes are just around double-quoting variables in the Once this lands, I'd like to eventually setup a TravisCI job that will run |
|
since she started it I'll let sinny finish the review tomorrow.. one positive note: it looks like shellcheck is in fedora: https://apps.fedoraproject.org/packages/ShellCheck |
Yup! I was running that in a container (obviously!) |
sinnykumari
left a comment
sinnykumari
left a comment
There was a problem hiding this comment.
One comment other than that looks good!
| coreos_gf_launch "$@" | ||
| coreos_gf run | ||
| local root=$(coreos_gf findfs-label root) | ||
| local root |
There was a problem hiding this comment.
Just curious, is there an advantage of declaring and defining local variable in separate line? If yes, maybe we should also change it at places like https://github.com/coreos/coreos-assembler/pull/193/files#diff-496abb20f29d0275d6697b14356e60e1R131
There was a problem hiding this comment.
Yeah, it's one of the things that shellcheck flags
https://github.com/koalaman/shellcheck/wiki/SC2155
Admittedly, I did notice that it didn't always flag what appeared to be "offending" code, but didn't dig into the why.
There was a problem hiding this comment.
I could be more thorough and find all the local assignments and change them to match, if that seems reasonable.
There was a problem hiding this comment.
I see, it's only a problem if one is executing a subprocess basically, in other words local vmpreparedir=${workdir}/tmp/supermin.prepare is fine.
There was a problem hiding this comment.
Ah, yup...that looks like the reasoning!
| # on the `mv`. | ||
| touch .build-commit | ||
| mv -T ${tmp_builddir} ${buildid} | ||
| mv -T "${tmp_builddir:?}" "${buildid}" |
There was a problem hiding this comment.
Hmm, why the explicit :? here? We're using set -u so that behavior applies everywhere right?
There was a problem hiding this comment.
(It doesn't hurt though, I'm fine leaving this as is)
There was a problem hiding this comment.
https://github.com/koalaman/shellcheck/wiki/SC2154
I used the guidance at the bottom:
If you know for a fact that the variable is set, you can use ${var:?} to fail if the variable is unset (or empty), or explicitly initialize/declare it with var="" or declare var.
There was a problem hiding this comment.
Ah, so if I have the shellcheck directive at the top of the file # shellcheck source=src/cmdlib.sh, this warning doesn't get thrown any more. Leaving the :? doesn't seem to hurt it either.
|
LGTM, @sinnykumari - do you want to do the merge? |
|
🎆 |
|
Sweet, this looks cool! |
5 participants
See https://github.com/koalaman/shellcheck for background