Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add shellcheck support and robustify present shell scripts #5294

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/shellcheck.yml
@@ -0,0 +1,26 @@
---
name: Shellcheck

on:
push:
branches: [develop]
pull_request:
branches: [develop]

permissions:
contents: read

jobs:
shellcheck:
name: Check shell scripts
runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v4
- name: Install dependencies
run: |
sudo apt update && sudo apt install -y shellcheck
- name: shellcheck
run: |
git grep -l '^#\( *shellcheck \|!\(/bin/\|/usr/bin/env \)\(sh\|bash\|dash\|ksh\)\)' | xargs shellcheck
Comment on lines +14 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love this!

Just to tidy up, do you think it makes sense to move this into the .github/workflows/lint.yml with the other lint job?

You'll have to move the permissions: contents: read into the jobs.shellcheck too.

1 change: 1 addition & 0 deletions .husky/pre-commit
@@ -1,4 +1,5 @@
#!/bin/sh
# shellcheck disable=SC1091
. "$(dirname "$0")/_/husky.sh"

pnpm run pre-commit
4 changes: 3 additions & 1 deletion docker-entrypoint.sh
@@ -1,3 +1,5 @@
#!/bin/sh
source /root/.shrc

# shellcheck disable=SC1091
. /root/.shrc
exec "$@"
50 changes: 28 additions & 22 deletions run
Expand Up @@ -8,14 +8,20 @@ underline() { ansi 4 "$@"; }
# strikethrough() { ansi 9 "$@"; }
red() { ansi 31 "$@"; }

name=$(basename $0)
name=$(basename "$0")
command=$1
# TODO: fix this one to become spaces tollerant etc
# If to be used as an array, make it
# args=( "${@:2}" )
# If to concatenated into a single string and thus loosing ability to pass args with spaces
# args="${*:2}"
# shellcheck disable=SC2124
args=${@:2}
Comment on lines +13 to 19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo(blocking): I'm fairly certain quoting "$args" will break this script.

Do you think it makes sense to change it to something like:

diff --git a/run b/run
index f01302c43..ecb392e58 100755
--- a/run
+++ b/run
@@ -10,18 +10,12 @@ red()           { ansi 31 "$@"; }
 
 name=$(basename "$0")
 command=$1
-# TODO: fix this one to become spaces tollerant etc
-#       If to be used as an array, make it
-# args=( "${@:2}" )
-#       If to concatenated into a single string and thus loosing ability to pass args with spaces
-# args="${*:2}"
-# shellcheck disable=SC2124
-args=${@:2}
+args=( "${@:2}" )
 
 case $command in
 
 build)
-docker compose build "$args"
+docker compose build "${args[@]}"
 ;;
 
 sh)
@@ -29,7 +23,7 @@ $RUN mermaid sh
 ;;
 
 pnpm)
-$RUN mermaid sh -c "pnpm $args"
+$RUN mermaid pnpm "${args[@]}"
 ;;
 
 dev)
@@ -41,7 +35,7 @@ $RUN --service-ports mermaid sh -c "pnpm run --filter mermaid docs:dev:docker"
 ;;
 
 cypress)
-$RUN cypress "$args"
+$RUN cypress "${args[@]}"
 ;;
 
 help|"")

I'll be honest, I don't really use this run script, but a quick test of the above changes seemed to work.


case $command in

build)
docker compose build $args
docker compose build "$args"
;;

sh)
Expand All @@ -35,7 +41,7 @@ $RUN --service-ports mermaid sh -c "pnpm run --filter mermaid docs:dev:docker"
;;

cypress)
$RUN cypress $args
$RUN cypress "$args"
;;

help|"")
Expand All @@ -52,41 +58,41 @@ ________________________________________________________________________________

Development Quick Start Guide:

$(bold ./$name pnpm install) # Install packages
$(bold ./$name dev) # Launch dev server with examples, open http://localhost:9000
$(bold ./$name docs:dev) # Launch official website, open http://localhost:3333
$(bold ./"$name" pnpm install) # Install packages
$(bold ./"$name" dev) # Launch dev server with examples, open http://localhost:9000
$(bold ./"$name" docs:dev) # Launch official website, open http://localhost:3333

$(bold ./$name pnpm vitest) # Run watcher for unit tests
$(bold ./$name cypress) # Run integration tests (after starting dev server)
$(bold ./$name pnpm build) # Prepare it for production
$(bold ./"$name" pnpm vitest) # Run watcher for unit tests
$(bold ./"$name" cypress) # Run integration tests (after starting dev server)
$(bold ./"$name" pnpm build) # Prepare it for production
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although looks odd, it is the only way to make it robust if script would ever be renamed to include space in the name. Alternatively that check could be skipped indeed.

__________________________________________________________________________________________

Commands:

$(bold ./$name build) # Build image
$(bold ./$name cypress) # Run integration tests
$(bold ./$name dev) # Run dev server with examples, open http://localhost:9000
$(bold ./$name docs:dev) # For docs contributions, open http://localhost:3333
$(bold ./$name help) # Show this help
$(bold ./$name pnpm) # Run any 'pnpm' command
$(bold ./$name sh) # Open 'sh' inside docker container for development
$(bold ./"$name" build) # Build image
$(bold ./"$name" cypress) # Run integration tests
$(bold ./"$name" dev) # Run dev server with examples, open http://localhost:9000
$(bold ./"$name" docs:dev) # For docs contributions, open http://localhost:3333
$(bold ./"$name" help) # Show this help
$(bold ./"$name" pnpm) # Run any 'pnpm' command
$(bold ./"$name" sh) # Open 'sh' inside docker container for development
__________________________________________________________________________________________

Examples of frequently used commands:

$(bold ./$name pnpm add --filter mermaid) $(underline package)
$(bold ./"$name" pnpm add --filter mermaid) $(underline package)
Add package to mermaid

$(bold ./$name pnpm -w run lint:fix)
$(bold ./"$name" pnpm -w run lint:fix)
Run prettier and ES lint

$(bold git diff --name-only develop \| xargs ./$name pnpm prettier --write)
$(bold git diff --name-only develop \| xargs ./"$name" pnpm prettier --write)
Prettify everything you added so far

$(bold ./$name cypress open --project .)
$(bold ./"$name" cypress open --project .)
Open cypress interactive GUI

$(bold ./$name cypress run --spec cypress/integration/rendering/)$(underline test.spec.ts)
$(bold ./"$name" cypress run --spec cypress/integration/rendering/)$(underline test.spec.ts)
Run specific test in cypress

$(bold xhost +local:)
Expand All @@ -99,7 +105,7 @@ echo -n -e "$usage"
;;

*)
message="$(red Unknown command: $command). See $(bold ./$name help) for available commands."
message="$(red Unknown command: "$command"). See $(bold ./"$name" help) for available commands."
echo -n -e "$message\n" >&2
$0 help
exit 1
Expand Down