wire-server icon indicating copy to clipboard operation
wire-server copied to clipboard

WPB-2568 Fix shellcheck linting problems on all shell scripts

Open smatting opened this issue 1 year ago • 4 comments
trafficstars

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

Checklist

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

smatting avatar Aug 20 '24 16:08 smatting

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 :facepalm:

akshaymankar avatar Aug 21 '24 07:08 akshaymankar

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

  1. Wait for https://github.com/numtide/treefmt/pull/389 to be merged and released and fly without treefmt for shell scripts until then
  2. Patch treefmt in nix with the bug fix

Thanks a lot for fixing all the shellscripts tho :heart:

akshaymankar avatar Aug 21 '24 08:08 akshaymankar

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

brianmcgee avatar Aug 21 '24 11:08 brianmcgee

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

smatting avatar Oct 21 '24 13:10 smatting