coreos-assembler icon indicating copy to clipboard operation
coreos-assembler copied to clipboard

shell: Drop most ignores for SC2031

Open cgwalters opened this issue 3 years ago • 5 comments

Followup to https://github.com/coreos/coreos-assembler/pull/3068

The original logic bug here was a classic shell script error, and I was really surprised why shellcheck didn't catch it - but instead we've had these ignores for SC2031 in other places.

I didn't fully debug this, but it looks like there's something going wrong in the analysis pass that's specific to the variable name "arch". Changing the variable name lets us omit the shellcheck ignore.

cgwalters avatar Nov 08 '22 13:11 cgwalters

I didn't fully debug this, but it looks like there's something going wrong in the analysis pass that's specific to the variable name "arch".

I'd be really interested in the root cause there since $arch is a pretty common shorthand for $architecture. Maybe a bug in shellcheck?

dustymabe avatar Nov 08 '22 14:11 dustymabe

I think the reason this happens in the case of create_disk.sh at least is that DEFAULT_TERMINAL=$(. "$(dirname "$0")"/cmdlib.sh; echo "$DEFAULT_TERMINAL") line. ShellCheck sees the arch= assignment in cmdlib.sh and the sourcing happens in a subshell so it's correct that "arch was modified in a subshell". In that case, a simpler approach is to move the DEFAULT_TERMINAL declaration higher up to before we declare our own arch variable.

jlebon avatar Nov 08 '22 15:11 jlebon

ShellCheck sees the arch= assignment in cmdlib.sh and the sourcing happens in a subshell so it's correct that "arch was modified in a subshell". In that case, a simpler approach is to move the DEFAULT_TERMINAL declaration higher up to before we declare our own arch variable.

Oh wow, yes. Good analysis. But I think the right fix here is to stop reading cmdlib.sh inside create_disk.sh at all, no? We should be passing this as input via the image.json I think.

Although actually AIUI, this whole thing is basically dead code now because we can rely on --platforms-json?

cgwalters avatar Nov 08 '22 20:11 cgwalters

Although actually AIUI, this whole thing is basically dead code now because we can rely on --platforms-json?

Not until the console changes reach FCOS stable next month.

bgilbert avatar Nov 08 '22 20:11 bgilbert

@cgwalters: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot avatar Nov 10 '22 08:11 openshift-merge-robot

Closing, cause create_disk.sh would go away soon

nikita-dubrovskii avatar Apr 18 '24 14:04 nikita-dubrovskii