Skip to content

Add ShellCheck to CI - #289

Merged
scal444 merged 2 commits into
NVIDIA-BioNeMo:mainfrom
scal444:issue-151-shellcheck-ci
Sep 10, 2026
Merged

Add ShellCheck to CI#289
scal444 merged 2 commits into
NVIDIA-BioNeMo:mainfrom
scal444:issue-151-shellcheck-ci

Conversation

@scal444

@scal444 scal444 commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Implements #151

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness or repository-rule violations identified.

Summary

  • Installs an integrity-checked ShellCheck 0.11.0 binary in the lint workflow.
  • Adds a repository-wide ShellCheck runner for tracked .sh and .bash files.
  • Improves quoting and null-safe file traversal across administration scripts.
  • Allows clang-tidy to continue when the optional LLVM symbolizer is unavailable.

Reviews (2) · Last reviewed commit: "Keep LLVM symbolizer optional in clang-t..."

Comment thread admin/run_clang_tidy.sh Outdated

export LLVM_SYMBOLIZER_PATH=$(which llvm-symbolizer-17)
LLVM_SYMBOLIZER_PATH=$(command -v llvm-symbolizer-17)
export LLVM_SYMBOLIZER_PATH

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With set -e on line 20, this now aborts the whole script if llvm-symbolizer-17 isn't installed. Looks like LLVM_SYMBOLIZER_PATH isn't read anywhere in this script. Do we really require it to be installed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. It's optional

@evasnow1992 evasnow1992 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes look good to me.

@scal444
scal444 merged commit fd0ec0d into NVIDIA-BioNeMo:main Sep 10, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants