WPB-2568 Fix shellcheck linting problems on all shell scripts by smatting · Pull Request #4220 · wireapp/wire-server

smatting · 2024-08-20T16:42:27Z

This PR activates shellcheck linting for almost all shell scripts.

It also fixes various suggestions on the shell scripts that are now shellchecked.

Tracked by https://wearezeta.atlassian.net/browse/WPB-2568

Current sprint: https://wearezeta.atlassian.net/browse/WPB-9757

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

akshaymankar · 2024-08-21T07:58:49Z

It is annoying that this wouldn't respect gitignore and could overwrite files in dist-newstyle, etc. I am looking into why it the git walker doesn't work. So far I have found this:

Running treefmt --no-cache -vv --walk=git -f shellcheck prints this:

DEBU format | shellcheck: executing: /home/axeman/workspace/wire-server/.env/bin/shellcheck docs/convert/convert.sh docs/convert/revert.sh hack/bin/helm_overrides.sh
INFO format | shellcheck: 3 file(s) processed in 21.992731ms
traversed 6499 files
emitted 6499 files for processing

But if we ask git about how many files it has by running git ls-files | wc -l, it says: 6671.

When using the filesystem walker it says this:

DEBU format | shellcheck: executing: /home/axeman/workspace/wire-server/.env/bin/shellcheck changelog.d/mk-cleanup.sh deploy/dockerephemeral/db-migrate/brig-index.sh deploy/dockerephemeral/db-migrate/brig-schema.sh deploy/dockerephemeral/db-migrate/galley-schema.sh deploy/dockerephemeral/db-migrate/gundeck-schema.sh deploy/dockerephemeral/db-migrate/spar-schema.sh deploy/dockerephemeral/init_vhosts.sh deploy/dockerephemeral/run.sh docs/convert/convert.sh docs/convert/revert.sh hack/bin/cabal-run-all-tests.sh hack/bin/cabal-run-integration.sh hack/bin/certchain.sh hack/bin/create_team.sh hack/bin/create_team_request_code.sh hack/bin/find-latest-docker-tag.sh hack/bin/gen-certs.sh hack/bin/generate-local-nix-packages.sh hack/bin/helm_overrides.sh hack/bin/integration-cleanup.sh hack/bin/integration-logs-relevant-bits.sh hack/bin/integration-test.sh hack/bin/kind-upload-image.sh hack/bin/kind-upload-images.sh hack/bin/kubectl-get-debug-info.sh hack/bin/mk-drv.sh hack/bin/nix-hls.sh hack/bin/oauth_test.sh hack/bin/register_idp.sh hack/bin/set-wire-server-image-version.sh hack/bin/update.sh hack/bin/upload-image.sh hack/bin/upload-images.sh integration/scripts/integration-dynamic-backends-brig-index.sh integration/scripts/integration-dynamic-backends-db-schemas.sh integration/scripts/integration-dynamic-backends-s3.sh integration/scripts/integration-dynamic-backends-ses.sh integration/scripts/integration-dynamic-backends-vhosts.sh libs/http2-manager/test/resources/gen-certs.sh services/background-worker/fill_rabbit.sh services/background-worker/shutdown_test.sh services/federator/test/resources/unit/gen-certs.sh services/federator/test/resources/unit/gen-multidomain-certs.sh services/gen-aws-conf.sh services/proxy/test/scripts/proxy-test.sh tools/hlint.sh
INFO format | shellcheck: 46 file(s) processed in 602.981369ms
traversed 41030 files
emitted 41030 files for processing
formatted 46 files (0 changed) in 1.517s

I didn't want us to find weird problems in future so I did a a timeboxed stare at treefmt's code and found that this code basically ignores any file which doesn't have the mode 100644, which means it excludes executable files causing our shellscripts to not be found 🤦

akshaymankar · 2024-08-21T08:18:36Z

Can we please not do the changes to treefmt args and instead do either of these things:

Wait for fix: Do not exclude executables in git walker numtide/treefmt#389 to be merged and released and fly without treefmt for shell scripts until then
Patch treefmt in nix with the bug fix

Thanks a lot for fixing all the shellscripts tho ❤️

brianmcgee · 2024-08-21T11:14:10Z

I cut a new release, which includes the fix for this: https://github.com/numtide/treefmt/releases/tag/v2.0.5.

You can wait until it propagates through Hydra, or add treefmt as a source (looks like you're using niv) and use the tag directly in the short-term: https://nixpkgs-tracker.ocfox.me/?pr=336307

hack/bin/cabal-run-tests.sh

julialongtin · 2024-09-23T10:35:47Z

hack/bin/create_test_user.sh

do you really need to enter and exit the quoted state for the commas here? i don't believe they will be considered part of the variable names...

Yep, correct, they are not needed. Fixed in 8a1940c

services/spar/test-scim-suite/run.sh

julialongtin

some of these changes with quotes look suspicious to me...

smatting · 2024-10-21T13:10:02Z

After rebasing on bumped nixpkgs we can use --walk=git to match all our scripts.

Randomly requested review from people I know that know bash

akshaymankar

✨

lwille

I might have found some issues, can you have a look?

lwille · 2024-10-31T15:21:03Z

hack/bin/serve-charts.sh

+SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
+cd "$SCRIPT_DIR/../../.local/charts"


thought: This seems a bit convoluted to me: we temporarily change directory just to print its name, then cd into that again.

suggestion: I think using dirname and realpath should do the same thing, are easier to understand and thus common practice:

Suggested change

SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

cd "$SCRIPT_DIR/../../.local/charts"

SCRIPT_DIR=$(realpath "$(dirname "${BASH_SOURCE[0]}")")

cd "$SCRIPT_DIR/../../.local/charts"

the solution is from https://stackoverflow.com/questions/59895/how-do-i-get-the-directory-where-a-bash-script-is-located-from-within-the-script It probably went through a lot of discussion, so I wouldn't wanna change it

tools/nginz_disco/nginz_disco.sh

lwille · 2024-10-31T15:38:03Z

tools/sftd_disco/sftd_disco.sh

    comma=""
    IFS=$'\n'
-    for entry in ${entries[@]}; do
+    for entry in "${entries[@]}"; do


issue: same thing here: the array is expanded inside the quotes, so you just get one entry with all records inside.

suggestion: don't quote ${entries[@]}, however use quoting when using $entry (as you did down the line).

Suggested change

for entry in "${entries[@]}"; do

for entry in ${entries[@]}; do

Thank you for checking! No, actually array quoted arrays are still splitting, and shellcheck suggests it https://www.shellcheck.net/wiki/SC2068
The problem here is that entries is not array, but a string. Applied your suggestsions in 88b16c8

echoes-hq bot added the echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. label Aug 20, 2024

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 20, 2024

smatting mentioned this pull request Aug 20, 2024

Fix shellcheck lint issues #4182

Closed

2 tasks

julialongtin reviewed Sep 23, 2024

View reviewed changes

hack/bin/cabal-run-tests.sh Outdated Show resolved Hide resolved

julialongtin reviewed Sep 23, 2024

View reviewed changes

services/spar/test-scim-suite/run.sh Outdated Show resolved Hide resolved

julialongtin requested changes Sep 23, 2024

View reviewed changes

smatting mentioned this pull request Sep 26, 2024

Bump nixpkgs #4273

Merged

2 tasks

smatting added 2 commits October 21, 2024 14:10

fix shellcheck problems

44cbb40

Add changelog entry

2ad0862

smatting force-pushed the WPB-2568-shellcheck-everything branch from d49b78b to 2ad0862 Compare October 21, 2024 12:12

smatting added 2 commits October 21, 2024 14:13

use --walk=git

b12553b

remove superfluous double quotes

8a1940c

smatting requested review from akshaymankar, julialongtin, lwille and supersven and removed request for supersven October 21, 2024 13:08

akshaymankar approved these changes Oct 31, 2024

View reviewed changes

lwille requested changes Oct 31, 2024

View reviewed changes

fix: these are note arrays

88b16c8

smatting requested a review from lwille November 4, 2024 09:54

smatting merged commit 2d98df4 into develop Nov 4, 2024

smatting deleted the WPB-2568-shellcheck-everything branch November 4, 2024 10:25

zebot mentioned this pull request Nov 4, 2024

Release 2024-11-04 - (expected chart version 5.7.0) #4327

Merged

		SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
		cd "$SCRIPT_DIR/../../.local/charts"

	for entry in "${entries[@]}"; do
	for entry in ${entries[@]}; do

Conversation

smatting commented Aug 20, 2024 • edited by lwille Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

akshaymankar commented Aug 21, 2024

Uh oh!

akshaymankar commented Aug 21, 2024

Uh oh!

brianmcgee commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

julialongtin Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

smatting Oct 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julialongtin left a comment

Choose a reason for hiding this comment

Uh oh!

smatting commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akshaymankar left a comment

Choose a reason for hiding this comment

Uh oh!

lwille left a comment

Choose a reason for hiding this comment

Uh oh!

lwille Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

smatting Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lwille Oct 31, 2024

Choose a reason for hiding this comment

Uh oh!

smatting Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

smatting commented Aug 20, 2024 •

edited by lwille

Loading

brianmcgee commented Aug 21, 2024 •

edited

Loading

smatting commented Oct 21, 2024 •

edited

Loading