bash scripts: snake-case variables, deduplicate $RANDOM, satisfy LSP #154
No reviewers
Labels
No labels
0 points
0.5 points
1 point
13 points
2 points
21 points
3 points
34 points
5 points
55 points
8 points
ambition
application-offering
ambition
configure-applications
ambition
front-end
ambition/install-applications
ambition
security
ambition
switch-host
ambition
update-applications
ambition
user-management
api service
blocked
component: fediversity panel
component: nixops4
documentation
estimation high: >3d
estimation low: <2h
estimation mid: <8h
infinite points
productisation
project-management
question
role: application developer
role: application operator
role: hosting provider
role: maintainer
security
technical debt
testing
type unclear
type: bug
type: deliverable
type: key result
type: objective
type: task
type: user story
user experience
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
fediversity/fediversity!154
Loading…
Reference in a new issue
No description provided.
Delete branch "kiara/fediversity:shellcheck"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
$(git log --pretty=%s | head -n 1)to bash scripts: snake-case variables, deduplicate $RANDOM, satisfy LSP@ -12,3 +12,3 @@readonly node=node051readonly tmpdir=/tmp/proxmox-provision-$RANDOM$RANDOMreadonly tmpdir=/tmp/proxmox-provision-$RANDOM$Trailing $
LGTM
7ff57a0518a0bbfd518fI feel most of what this PR does is making happy a chatty LSP at the cost of human readability.
The three actual changes:
$RANDOM-rtoreadsnake_caseinstead oflowercaseseem all fine although not particularly important.
The PR introduces at least two bugs (the
diething and thereadonlything) and there are too many changes for me to be confident that it didn't introduce others. Let's fix those few bugs, then test it the afternoon, @kiara?@ -1,17 +1,17 @@#!/usr/bin/env sh#!/usr/bin/env bashThe whole script is POSIX-compliant, so why make it a bash script here?
the LSP had it
$RANDOMis not POSIX-compliantOh nice, I learned something today. Still feels a bit sad to turn a whole script to bash, but that now sounds like the thing to do!
@ -12,3 +12,3 @@readonly node=node051readonly tmpdir=/tmp/proxmox-provision-$RANDOM$RANDOMreadonly tmpdir=/tmp/proxmox-provision-$RANDOM$I put two initially for increased entropy, but I suppose that's overkill and one is more than enough. Without the trailing
$though.@ -49,2 +49,2 @@die () { printf '\033[31m'; printf "$@"; printf '\033[0m\n'; exit 2; }die_with_help () { printf '\033[31m'; printf "$@"; printf '\033[0m\n'; help; exit 2; }die () { printf '\033[31m'; printf "%s" "$@"; printf '\033[0m\n'; exit 2; }die_with_help () { printf '\033[31m'; printf "%s" "$@"; printf '\033[0m\n'; help; exit 2; }No, this is changing the semantics in the wrong way. The goal is precisely to pass the arguments of
dietoprintfdirectly so that we can do eg.die '%s %d' foo 2. This also risks passing many more arguments toprintf. Please revert those two lines.@ -56,2 +56,2 @@--username) readonly username=$1; shift ;;--password) readonly password=$1; shift ;;--username) readonly username="$1"; shift ;;--password) readonly password="$1"; shift ;;While adding
"can in general make things safer and avoids needing global reasoning, adding them here is just useless in all situations; word splitting does not happen in an assignment word. If the suggestion comes from the LSP, this is very wrong.Re this one, the handling of the arguments of the
readonlybuilt-in seems to differ between Shell implementations. The POSIX standard seems to suggest that word expansion should not happen, but it isn't extremely clear.More importantly, standard or not, if
dashandbashexhibit different behaviours, we should avoid it altogether. Quoting sounds like the right solution here.@ -63,3 +63,3 @@-h|-\?|--help) help; exit 0 ;;-*) die_with_help 'Unknown argument: `%s`.' "$argument" ;;-*) die_with_help "Unknown argument: \`\%s\`." "$argument" ;;I find this sad.
I feel you.
I understand the advice's point about ensuring we do not accidentally hard-code bash-y stuff.
I could instead disable the particular check for this line with a comment.
Would you find that preferable for these cases?
Is there a risk of something bash-y? I thought the advice was just
which is fair depending on the LSP's target, I suppose. Also, I didn't notice the first time, but do we need to escape
%?I suppose a compromise that should make your LSP happy while remaining readable is to go for
WDYT?
apologies for not having come up with that - that one seems much nicer alright!
@ -74,2 +73,3 @@{ read -r username; read -r password; } < .proxmoxelsedie_with_help 'Required: `--username` and `--password`.\n'die_with_help "Required: \`--username\` and \`--password\`.\n"I find this sad.
@ -105,0 +103,4 @@readonly ticketticket=$(echo "$response" | jq -r .data.ticket)readonly csrfTokencsrfToken=$(echo "$response" | jq -r .data.CSRFPreventionToken)I'm curious where the change comes from. Making this variables read only before writing to them is just not going to work!
Also, if we're going for
snake_case, we might as well be consistent and usecsrf_token.Just like in Fediversity/Fediversity#154 (comment), I believe this change makes sense. It is still wrong to have the
readonlyline before the assignment, but we should eitherreadonly Xafter the assignment toX$( ... )subshellthanks for catching the casing inconsistency. i do see some discussion on casing leaning toward snake-case for bash variable names as well, so fair enough. i pushed to reflect that now.
I don't have any particular opinion on casing in Shell. I guess I have seen often
snake_caseindeed, so maybe that's fairly standard? Either way, it makes sense to try and stick to one style.thanks, looks like i should not have assumed bash to work like TypeScript here.
unfortunately, just quoting the assignment appears not to satisfy the rule (SC2155 Declare and assign separately to avoid masking return values).
i'll put the
readonlystatements after assignment then, as you suggested.@ -109,3 +111,3 @@}release_lock () {rmdir $tmpdir/lock-$1rmdir "$tmpdir/lock-$1"Now we're entering the realm where adding quotes makes sense for local reasoning. Like we know that
$tmpdirand$1do not contain spaces but that's only with global reasoning. I understand a linter might complain and I guess this might also make us a bit more future proof; let's keep those.@ -137,3 +139,3 @@running) sleep 1 ;;stopped) break ;;*) die 'unexpected status: `%s`' "$status" ;;*) die "unexpected status: \`\%s\`" "$status" ;;Also sad.
@ -234,3 +236,3 @@running) sleep 1 ;;stopped) break ;;*) printf ' unexpected status: `%s`\n' "$status"; exit 2 ;;*) printf " unexpected status: \`\%s\`\n" "$status"; exit 2 ;;Sad.
@ -275,2 +277,2 @@for vmid in $vmids; doprovision_vm $vmid &for vm_id in $vm_ids; doprovision_vm "$vm_id" &This is where we know that
$vm_idis safe and never needs to be quoted, because it comes from aforloop, and that's why all the$1above didn't need to be quoted also.Sure - I don't doubt you had already verified any present behavior.
Now, given LSP hints that may be overly careful, our options seem:
in this PR i opted for following the advice - would you prefer another option in cases like this one?
My preference would be 5, but I don't think it would be a good use of our time. I'm just annoyed by the noise and poor quality of Shell tooling, and LSPs in particular. (I spent 5 years of my life writing static and dynamic analysers for Shell, so this is a problem I have at heart!)
Let's try to do 1., follow the advice to reduce LSP noise: some of the LSP suggestions were actually catching interesting things (eg. the
readonlystuff), so it would be a shame to drown this under the rest. However, some suggestions are just plain wrong, so these can obviously not make the cut, and I would try to find ways to get around the LSP's suggestions sometimes without hindering readability (eg. with the'..`..`..'thing).So let's try to fix what needs to be fixed in this PR and merge it!
@ -1,17 +1,17 @@#!/usr/bin/env sh#!/usr/bin/env bashSame as other file.
@ -41,2 +41,2 @@die () { printf '\033[31m'; printf "$@"; printf '\033[0m\n'; exit 2; }die_with_help () { printf '\033[31m'; printf "$@"; printf '\033[0m\n'; help; exit 2; }die () { printf '\033[31m'; printf "%s" "$@"; printf '\033[0m\n'; exit 2; }die_with_help () { printf '\033[31m'; printf "%s" "$@"; printf '\033[0m\n'; help; exit 2; }Same as other file.
@ -51,3 +51,3 @@-h|-\?|--help) help; exit 0 ;;-*) die_with_help 'Unknown argument: `%s`.' "$argument" ;;-*) die_with_help "Unknown argument: \`\%s\`." "$argument" ;;Sad times.
@ -62,2 +61,3 @@{ read -r username; read -r password; } < .proxmoxelsedie_with_help 'Required: `--username` and `--password`.\n'die_with_help "Required: \`--username\` and \`--password\`.\n"Sad times.
@ -80,0 +78,4 @@readonly ticketticket=$(echo "$response" | jq -r .data.ticket)readonly csrfTokencsrfToken=$(echo "$response" | jq -r .data.CSRFPreventionToken)Same as other file.
@ -111,3 +113,3 @@running) sleep 1 ;;stopped) break ;;*) die 'unexpected status: `%s`' "$status" ;;*) die "unexpected status: \`\%s\`" "$status" ;;Sad times.
a0bbfd518fc1e51e6323c1e51e632389ec24d53489ec24d5343e342005943e3420059420493966b020493966b0eeed642dd4eeed642dd492a7e66900@Niols thanks again for all the feedback - let's do that!
92a7e669003addd3fd1a3addd3fd1a1ab3cbe5dbRebased on current
main, tracked down and fixed issues. Now it seems to work fine, I have tested it successfully. @kiara> Happy with the new commits?1ab3cbe5db18a14d29ab(Rebased again, we merge things fast in this country :p)
@Niols looks great, thank you! 🙈