coreos / fedora-coreos-docs

Documentation for Fedora CoreOS
https://docs.fedoraproject.org/en-US/fedora-coreos/
Other
49 stars 119 forks source link

lint: fix shellcheck findings in embedded shell scripts #552

Open Okeanos opened 1 year ago

Okeanos commented 1 year ago

vmware:

travier commented 1 year ago

That looks great! Thanks a lot. I'll do an deeper review soon.

Okeanos commented 1 year ago

Hmm. I'm a bit torn here. While I see value in making things more correct and observing good practices I also feel like a lot of these changes make things harder to read. All the extra quoting and brackets disrupt the flow as you are reading the docs and makes the lines go longer which means users will more often need to scroll.

Should we focus our efforts here on cases where there is a higher chance of one of these problems happening?

I absolutely understand and I definitely see the value and validity in your review findings. Given the necessity to extract the shell blocks from the asciidoc surroundings to be able to use shellshock at all, loss of context was inevitable and a certainly lead to being overaggressive in seeing & applying suggestions.

I'll go over your findings, fix them – especially the quotes surrounding $() blocks (I really shouldn't have applied these in the first place … d'oh) for example – and then we'll see what else can be made nicer to read.

travier commented 1 year ago

Let's focus on the cases where there is a potential for issue (space in file path for example) first and we'll see what to do for the other ones later.

Okeanos commented 1 year ago

Let's focus on the cases where there is a potential for issue (space in file path for example) first and we'll see what to do for the other ones later.

I think I removed most if not all quotes and additional curly braces where I could reasonably deduce/assume that the variable's value in question is very much unlikely to contain whitespace characters (or other shell-relevant characters like #).

At the same time I took the liberty (duck) of making the coreos-installer invocations reasonably consistent across the pages to offer a more consistent reading experience. I can revert those though if it makes the change set too noisy or is deemed frivolous.

The virt-install and qemu-kvm commands are a little tricky – we put (sometimes nested multiple times and then unwrapped at invocation time) variables in them that will absolutely contain path expressions (such as ${PWD}) that are likely to contain spaces themselves. That's quoting hell in a nutshell. While I am tempted to "fix" this … I am not sure it's worth it given that these commands are likely not going to be executed by novices who'll stumble across this. I am overall just as willing to forgo quotes altogether here to not make it even weirder.

Please don't hesitate to call out anything else you want changed – I essentially took it upon myself to do this so no hard feeling if you want it differently or not at all.