Run shellcheck as a github workflow

[hulthe]

shellcheck lints scripts for common footguns. This PR adds shellcheck as a GitHub action, and also attempts to fix all the warnings we currently have.

Scripts are finnicky, and shellcheck has a tendency to give false positives, so the changed scripts should be tested before merging if the changes are non-trivial.

---

This change is 

[linear]

DES-593 Run shellcheck in CI

[hulthe]

https://www.shellcheck.net/wiki/SC2128

[hulthe]

https://www.shellcheck.net/wiki/SC2086

[faern]

Some of the scripts (most notably the iOS scripts) I don't feel like I can verify correctly. Can you please have someone in the iOS team test these changes out and verify they work as expected? I know we have previously had issuse where shellcheck recommendations break iOS scripts.

Reviewed 7 of 12 files at r1.
Reviewable status: 7 of 12 files reviewed, 16 unresolved discussions (waiting on @hulthe)

---

.github/workflows/shellcheck.yml line 5 at r1 (raw file):

  push:
    branches:
      - main

Is this needed on push? We don't run other CI jobs on push really, except a few, but they have special reasons. As long as we check all PRs, main should never get corrupted. I'm not saying I'm completely against this, just that it's different from the pattern we usually have. I'm open to suggested improvements.

---

.github/workflows/shellcheck.yml line 8 at r1 (raw file):

name: "Trigger: Push action"
permissions: {}

Are these two lines needed? we don't really specify them for other jobs. I think this will work fine without? 🤔

---

.github/workflows/shellcheck.yml line 17 at r1 (raw file):

      - uses: actions/checkout@v3
      - name: Run ShellCheck
        uses: ludeeus/action-shellcheck@master

Would it make sense to pin this to a release of the action instead of master? Mostly to stop sudden breakage. The CI fails often enough without depending on moving targets 😂 Seems like 2.0.0 is the latest release.

---

.github/workflows/shellcheck.yml line 20 at r1 (raw file):

        with:
          ignore_names: >-
            gradlew

We really only have a single gradlew file that we want to ignore. So it feels like we can specify this more exactly with:

ignore_paths: >-
  android/gradlew

I feel like this more exactly express what we want to ignore.

[faern]

Reviewed 1 of 12 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 8 of 12 files reviewed, 12 unresolved discussions (waiting on @hulthe)

---

wireguard/libwg/build-android.sh line 55 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

note: I assume this is not what the author intended. Worth fixing?

Will this really fix the "issue"? This will still only chmod the deepest directory. I assume the previous code worked, since we already use it 🤔

[faern]

Reviewed 1 of 1 files at r3.
Reviewable status: 8 of 12 files reviewed, 11 unresolved discussions

[buggmagnet]

Reviewed 10 of 12 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @hulthe)

---

ci/ios/buildserver-build-ios.sh line 96 at r3 (raw file):

        run_git fetch --prune --tags 2> /dev/null || continue
        local tags
        mapfile -t tags < <(run_git tag | grep "$TAG_PATTERN_TO_BUILD")

bash-3.2$ mapfile 
bash: mapfile: command not found

It looks like this builtin does not exists on bash for macOS. I think we should revert this change.

---

ci/ios/run-in-vm.sh line 27 at r3 (raw file):

# apparently, there's a difference between piping into zsh like this and doing
# a <(echo $SCRIPT).
ssh admin@"$(tart ip "$VM_NAME")" bash /dev/stdin < "$SCRIPT"

We can remove the above comment as it will be invalid after this change.
I just tested this, and that seems to work, so this change should be fine.

---

ios/build-wireguard-go.sh line 41 at r3 (raw file):

# Run make
/usr/bin/make -C "$WIREGUARD_KIT_GO_PATH" "$ACTION"

This is intentionally done without quotes, this script is ran from within Xcode, and it won't always have an argument.

If we add quotes here, we will pass an empty string as an argument which will make our build fail, please revert this change.

[hulthe]

Reviewable status: 9 of 12 files reviewed, 14 unresolved discussions (waiting on @buggmagnet)

---

ci/ios/buildserver-build-ios.sh line 96 at r3 (raw file):

Previously, buggmagnet wrote…

bash-3.2$ mapfile 
bash: mapfile: command not found

It looks like this builtin does not exists on bash for macOS. I think we should revert this change.

Done.

---

ci/ios/run-in-vm.sh line 27 at r3 (raw file):

Previously, buggmagnet wrote…

We can remove the above comment as it will be invalid after this change.
I just tested this, and that seems to work, so this change should be fine.

Done.

---

ios/build-wireguard-go.sh line 41 at r3 (raw file):

Previously, buggmagnet wrote…

This is intentionally done without quotes, this script is ran from within Xcode, and it won't always have an argument.

If we add quotes here, we will pass an empty string as an argument which will make our build fail, please revert this change.

Done.

[buggmagnet]

My review pertains only to iOS specific changes, but
Thank you for doing this!

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions

[faern]

Reviewed 1 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions