Skip to content

validate shell scripts with shellcheck#312

Merged
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-shellcheck
Jun 18, 2026
Merged

validate shell scripts with shellcheck#312
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-shellcheck

Conversation

@bennyz

@bennyz bennyz commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • CI/CD improvement
  • Refactoring

Testing

  • Unit tests pass (make test)
  • Linter passes (make lint)
  • Manifests are up to date (make manifests generate)
  • Tested on OpenShift cluster (if applicable)

Summary by CodeRabbit

  • Chores
    • Introduced shell script linting validation to the continuous integration pipeline to identify and prevent issues earlier.
    • Enhanced robustness of internal build and deployment scripts through improved error handling, defensive directory navigation, and consistent quoting practices.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4ea160a7-fac7-4c69-97a3-d51747bd43c6

📥 Commits

Reviewing files that changed from the base of the PR and between d89c0d7 and 133881f.

📒 Files selected for processing (11)
  • .github/workflows/lint.yml
  • Makefile
  • hack/create-release-branch.sh
  • hack/deploy-catalog.sh
  • hack/rebuild-catalog.sh
  • internal/common/tasks/scripts/build_builder.sh
  • internal/common/tasks/scripts/build_image.sh
  • internal/common/tasks/scripts/common.sh
  • internal/common/tasks/scripts/find_manifest.sh
  • internal/common/tasks/scripts/flash_image.sh
  • internal/common/tasks/scripts/push_artifact.sh

📝 Walkthrough

Walkthrough

Adds a lint-shell Makefile target running shellcheck --severity=warning over all .sh files, wires it into the GitHub Actions lint job, and remediates the resulting warnings across hack/ and internal/common/tasks/scripts/ shell scripts via quoting fixes, defensive || exit guards, read -ra array splitting, unused variable renaming, shellcheck directives, and printf over echo -n.

Changes

ShellCheck Linting Infrastructure and Script Fixes

Layer / File(s) Summary
ShellCheck Makefile target and CI workflow step
Makefile, .github/workflows/lint.yml
Adds the phony lint-shell target invoking shellcheck --severity=warning on all *.sh files (excluding .git/ and vendor/), and adds a make lint-shell step to the lint CI job.
ShellCheck fixes in hack/ scripts
hack/create-release-branch.sh, hack/deploy-catalog.sh, hack/rebuild-catalog.sh
Renames the unused version split variable to _PATCH; replaces named loop index variables with unused placeholders in three polling loops and the registry-route wait loop; quotes oc whoami/token command substitutions in registry login calls; switches ARCHS_ARRAY initialization from bare word-splitting to read -ra with a here-string.
ShellCheck directives and defensive fixes in task scripts
internal/common/tasks/scripts/build_builder.sh, internal/common/tasks/scripts/build_image.sh, internal/common/tasks/scripts/common.sh, internal/common/tasks/scripts/find_manifest.sh, internal/common/tasks/scripts/flash_image.sh, internal/common/tasks/scripts/push_artifact.sh
Adds # shellcheck shell=bash headers to four scripts; adds shellcheck disable comments for cross-script variables NAMESPACE and GET_SIZE_CMD in common.sh; adds || exit guards to cd/pushd calls in build_image.sh and push_artifact.sh; uses ${VAR:?} expansion and quotes ln -sf arguments in build_image.sh artifact finalization; rewrites find with a grouped -o expression and replaces echo -n with printf '%s' in find_manifest.sh.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • bkhizgiy

Poem

🐇 A rabbit once checked every shell,
With shellcheck to guard each command well.
|| exit guards in place,
Quoted vars with grace,
Now the scripts run without a misspell! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'validate shell scripts with shellcheck' accurately and clearly summarizes the main change—introducing shell script validation via shellcheck to the CI/CD pipeline.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bennyz

bennyz commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

@ambient-code please review

@ambient-code ambient-code Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good PR -- adding shellcheck validation and fixing the warnings it surfaces is a solid improvement. The quoting fixes, || exit guards on cd/pushd/popd, unused loop variable renames, and ${WORKSPACE_PATH:?} guard on rm -rf are all correct and valuable.

I left a few comments, mostly minor. The find precedence bug in find_manifest.sh is the most impactful one.

Comment thread internal/common/tasks/scripts/find_manifest.sh Outdated
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread internal/common/tasks/scripts/build_image.sh Outdated
Comment thread internal/common/tasks/scripts/build_image.sh Outdated
Comment thread .github/workflows/lint.yml
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-opus-4.6
@bennyz bennyz force-pushed the worktree-shellcheck branch from 9709c7e to 133881f Compare June 16, 2026 11:11
@bennyz bennyz requested review from bkhizgiy and maboras-rh June 18, 2026 12:40
@bennyz bennyz merged commit 89a9c1e into centos-automotive-suite:main Jun 18, 2026
4 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