chore: add shellcheck to lint .sh files

[tamirazrab]

Description

Add shellcheck to project, - couldn't make it to fix sh files automatically.
...

End to End Test:
(See Pepr Excellent Examples)

Related Issue

Fixes #1706

Relates to #

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Other (security config, docs update, etc)

Checklist before merging

• [] Unit, Journey, E2E Tests, docs, adr added or updated as needed
• Contributor Guide Steps followed

[samayer12]

@tamirazrab, appreciate the PR! We can't guarantee that local developers have shellcheck installed, so it's possible that a contributor would have the linter fail unless we add shellcheck as a dev dependency.

Once that's done, we should be able to run shellcheck within the project and avoid that issue.

While we're at it, I also resolved a shellcheck warning in integration/prep.sh.

[samayer12]

As a note, a survey of the project reveals two *.sh files. Command used:

find . -iname "*.sh" -not -path "./node_modules/*" -not -path "./.husky/_/*"

We also have 12 run: blocks within our github actions. Some of these are one-liners, other may be larger and could be broken out into test scripts. Regardless, that work is outside the scope of this PR.

[samayer12]

I also noticed that the shellcheck pattern did not include the husky hooks we have configured in .husky. So, I've updated .linstagedrc.json to account for them. Those files were sneaky since they didn't have a *.sh suffix.

@tamirazrab, we also need to switch the shellcheck format so that errors could print in the terminal. Can you confirm that using shellcheck -f tty works as we'd expect? For example, we could create a .sh file with some intentional shellcheck issues, try to commit it, and hopefully see error messages in the terminal output. As a result, the file should not be committed as long as there is an error.

[tamirazrab]

As a note, a survey of the project reveals two *.sh files. Command used:

find . -iname "*.sh" -not -path "./node_modules/*" -not -path "./.husky/_/*"

We also have 12 run: blocks within our github actions. Some of these are one-liners, other may be larger and could be broken out into test scripts. Regardless, that work is outside the scope of this PR.

Definitely do able for larger parts.

[tamirazrab]

I also noticed that the shellcheck pattern did not include the husky hooks we have configured in .husky. So, I've updated .linstagedrc.json to account for them. Those files were sneaky since they didn't have a *.sh suffix.

@tamirazrab, we also need to switch the shellcheck format so that errors could print in the terminal. Can you confirm that using shellcheck -f tty works as we'd expect? For example, we could create a .sh file with some intentional shellcheck issues, try to commit it, and hopefully see error messages in the terminal output. As a result, the file should not be committed as long as there is an error.

Good catch they should've been included, by your change it doesn't seem to work at my end so I separated them as different steps in .linstagedrc.json. I can see linting error messages clearly on terminal. Not sure why but commits hooks aren't working on my end. I manually ran npx lint-staged --verbose to confirm and resolved some warns for pre-push.

[samayer12]

Not sure why but commits hooks aren't working on my end. I manually ran npx lint-staged --verbose to confirm and resolved some warns for pre-push.

Just to double-check, have you ran npm install from within the project directory? I ran the following commands and noticed that the commit hooks don't fire without it (since husky is a devDependency). Husky docs, for reference.

  rm -rf node_modules/
  touch file.txt
  git add file.txt
  git commit -m "testing - pre-install"
  # No commit hook runs

Then, reset repo state with git reset --hard HEAD~1.

  npm i
  touch file.txt
  git add file.txt
  git commit -m "testing post-install" 
  # Commit hook runs

[tamirazrab]

Not sure why but commits hooks aren't working on my end. I manually ran npx lint-staged --verbose to confirm and resolved some warns for pre-push.

Just to double-check, have you ran npm install from within the project directory? I ran the following commands and noticed that the commit hooks don't fire without it (since husky is a devDependency). Husky docs, for reference.

  rm -rf node_modules/
  touch file.txt
  git add file.txt
  git commit -m "testing - pre-install"
  # No commit hook runs

Then, reset repo state with git reset --hard HEAD~1.

  npm i
  touch file.txt
  git add file.txt
  git commit -m "testing post-install" 
  # Commit hook runs

Had node_modules installed before, reinstalled now hooks are firing, thanks.

[samayer12]

@tamirazrab it looks like commit b402851 wasn't signed. We'll need to have all commits in the PR signed to follow org policy.

[tamirazrab]

@tamirazrab it looks like commit b402851 wasn't signed. We'll need to have all commits in the PR signed to follow org policy.

Yeah so I forgot to sign that commit, when I again signed it, it popped as new commit e745fa8.

[samayer12]

@tamirazrab, I see the signed commit later in the chain. One option here would be to retroactively sign the commits through an interactive rebase (reference) and then do a git push --force-with-lease so that the branch's entire history is signed.

Additionally, it looks like CI is failing because of a mismatch between the package.json and package-lock.json file. When adding shellcheck, I bet your local copy of the package-lock.json was modified and should be committed.

[socket-security]

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/[email protected]	environment Transitive: filesystem, network, shell	+72	1.73 MB	gunar

View full report↗︎

[tamirazrab]

@tamirazrab, I see the signed commit later in the chain. One option here would be to retroactively sign the commits through an interactive rebase (reference) and then do a git push --force-with-lease so that the branch's entire history is signed.

Additionally, it looks like CI is failing because of a mismatch between the package.json and package-lock.json file. When adding shellcheck, I bet your local copy of the package-lock.json was modified and should be committed.

Thanks for the link, actually after some time I couldn't do it not sure why (maybe need to practice it in some demo-repo) so instead I actually dropped that commit, now all commits seems signed.

Yes you are right, at work we avoid our local copy of lock file during commits, I thought it might be the same case here 😅, anyhow I added it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.76%. Comparing base (6e131ce) to head (40d0c0b).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1730   +/-   ##
=======================================
  Coverage   79.76%   79.76%           
=======================================
  Files          50       50           
  Lines        2120     2120           
  Branches      431      453   +22     
=======================================
  Hits         1691     1691           
+ Misses        427      401   -26     
- Partials        2       28   +26     

see 8 files with indirect coverage changes