-
Notifications
You must be signed in to change notification settings - Fork 120
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
lint: fix shellcheck findings in embedded shell scripts #552
base: main
Are you sure you want to change the base?
Conversation
That looks great! Thanks a lot. I'll do an deeper review soon. |
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.
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 |
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. |
4702c3b
to
27ff67a
Compare
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 The 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. |
27ff67a
to
66e64a7
Compare
- prevent wordsplitting and accidental globbling by quoting variables - use parameter expansion with braces for consistency where prudent - remove a number of useless cat-expressions - replace printf with echo for fewer differences across examples - align coreos-installer snippets for a more consistent reading experience vmware: - use correct gzip file ending (See coreos#524 (comment) for details) in example
66e64a7
to
2f17ceb
Compare
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.
This looks like a good overall improvement.
Thanks for working on it!
I'll give some time for others to have a look before merging.
vmware: