fix: Fixes issue with script not running correctly in zsh enviornment.#36
Open
dlabaj wants to merge 1 commit intopatternfly:mainfrom
Open
fix: Fixes issue with script not running correctly in zsh enviornment.#36dlabaj wants to merge 1 commit intopatternfly:mainfrom
dlabaj wants to merge 1 commit intopatternfly:mainfrom
Conversation
WalkthroughEnhanced the installation script to enforce Bash execution and improve zsh shell compatibility. Added automatic NVM configuration setup for zsh users and refined user-facing output messages with corrected text and post-installation sourcing instructions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/install.sh (1)
5-8: The documented install pattern usesbashdirectly, so the guard will not break it—but improve error messaging for accidental non-bash invocation.The README correctly documents
curl -fsSL ... | bash, which meansBASH_VERSIONis set and the guard does not trigger. However, if a user accidentally runscurl ... | zshorcurl ... | sh, the guard would fail confusingly because$0contains the shell name rather than a file path. Consider adding a check for stdin invocation to provide a clearer error message:# Ensure this script runs in bash, even if invoked with zsh if [ -z "${BASH_VERSION:-}" ]; then + if [ ! -f "$0" ]; then + printf '\nERROR: This installer must be run with bash. Download it first, then run: bash scripts/install.sh\n\n' >&2 + exit 1 + fi exec bash "$0" "$@" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install.sh` around lines 5 - 8, The current guard uses BASH_VERSION and exec bash "$0" "$@" which can produce confusing behavior when the script is piped into a non-bash shell because $0 is the shell name; update the check so when BASH_VERSION is empty you detect piped/stdin invocation (e.g., $0 is "-" or not a regular file or contains "sh"/"zsh") and, if so, print a clear error to stderr instructing the user to run the installer with bash (for example: "This installer must be run with bash. Use: curl ... | bash") and exit non‑zero; otherwise, preserve the exec bash "$0" "$@" fallback so direct non‑bash executions still re‑exec under bash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install.sh`:
- Around line 70-86: The zsh snippet in scripts/install.sh currently hardcodes
export NVM_DIR="$HOME/.nvm", which breaks custom installs; update the code that
appends to the zshrc (the block that writes to the variable zshrc) so it
persists the actual NVM_DIR value instead of hardcoding ~/.nvm—write the export
using parameter expansion to prefer an existing environment NVM_DIR and fall
back to $HOME/.nvm (mirror the behavior used by ensure_nvm_loaded), and keep the
subsequent checks that source nvm.sh and bash_completion unchanged.
---
Nitpick comments:
In `@scripts/install.sh`:
- Around line 5-8: The current guard uses BASH_VERSION and exec bash "$0" "$@"
which can produce confusing behavior when the script is piped into a non-bash
shell because $0 is the shell name; update the check so when BASH_VERSION is
empty you detect piped/stdin invocation (e.g., $0 is "-" or not a regular file
or contains "sh"/"zsh") and, if so, print a clear error to stderr instructing
the user to run the installer with bash (for example: "This installer must be
run with bash. Use: curl ... | bash") and exit non‑zero; otherwise, preserve the
exec bash "$0" "$@" fallback so direct non‑bash executions still re‑exec under
bash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+70
to
+86
| local zshrc="${HOME}/.zshrc" | ||
| if [ ! -f "$zshrc" ]; then | ||
| touch "$zshrc" | ||
| fi | ||
|
|
||
| if grep -q 'NVM_DIR' "$zshrc" 2>/dev/null; then | ||
| info "nvm is already configured in ${zshrc}" | ||
| return 0 | ||
| fi | ||
|
|
||
| info "Adding nvm to ${zshrc} for zsh compatibility." | ||
| cat >> "$zshrc" << 'EOF' | ||
|
|
||
| # Load nvm (Node Version Manager) | ||
| export NVM_DIR="$HOME/.nvm" | ||
| [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" | ||
| [ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion" |
There was a problem hiding this comment.
Persist the actual NVM_DIR instead of hardcoding ~/.nvm.
ensure_nvm_loaded supports custom NVM_DIR, but the zsh snippet always writes export NVM_DIR="$HOME/.nvm". Users installing nvm into a custom location will get a broken future zsh session.
Proposed fix
- local zshrc="${HOME}/.zshrc"
+ local zshrc="${HOME}/.zshrc"
+ local nvm_dir="${NVM_DIR:-$HOME/.nvm}"
@@
- cat >> "$zshrc" << 'EOF'
+ cat >> "$zshrc" << EOF
# Load nvm (Node Version Manager)
-export NVM_DIR="$HOME/.nvm"
-[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"
-[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"
+export NVM_DIR="${nvm_dir}"
+[ -s "\$NVM_DIR/nvm.sh" ] && \. "\$NVM_DIR/nvm.sh"
+[ -s "\$NVM_DIR/bash_completion" ] && \. "\$NVM_DIR/bash_completion"
EOF📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local zshrc="${HOME}/.zshrc" | |
| if [ ! -f "$zshrc" ]; then | |
| touch "$zshrc" | |
| fi | |
| if grep -q 'NVM_DIR' "$zshrc" 2>/dev/null; then | |
| info "nvm is already configured in ${zshrc}" | |
| return 0 | |
| fi | |
| info "Adding nvm to ${zshrc} for zsh compatibility." | |
| cat >> "$zshrc" << 'EOF' | |
| # Load nvm (Node Version Manager) | |
| export NVM_DIR="$HOME/.nvm" | |
| [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" | |
| [ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion" | |
| local zshrc="${HOME}/.zshrc" | |
| local nvm_dir="${NVM_DIR:-$HOME/.nvm}" | |
| if [ ! -f "$zshrc" ]; then | |
| touch "$zshrc" | |
| fi | |
| if grep -q 'NVM_DIR' "$zshrc" 2>/dev/null; then | |
| info "nvm is already configured in ${zshrc}" | |
| return 0 | |
| fi | |
| info "Adding nvm to ${zshrc} for zsh compatibility." | |
| cat >> "$zshrc" << EOF | |
| # Load nvm (Node Version Manager) | |
| export NVM_DIR="${nvm_dir}" | |
| [ -s "\$NVM_DIR/nvm.sh" ] && \. "\$NVM_DIR/nvm.sh" | |
| [ -s "\$NVM_DIR/bash_completion" ] && \. "\$NVM_DIR/bash_completion" | |
| EOF |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.sh` around lines 70 - 86, The zsh snippet in
scripts/install.sh currently hardcodes export NVM_DIR="$HOME/.nvm", which breaks
custom installs; update the code that appends to the zshrc (the block that
writes to the variable zshrc) so it persists the actual NVM_DIR value instead of
hardcoding ~/.nvm—write the export using parameter expansion to prefer an
existing environment NVM_DIR and fall back to $HOME/.nvm (mirror the behavior
used by ensure_nvm_loaded), and keep the subsequent checks that source nvm.sh
and bash_completion unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This resolves jira issue #4005 . The install script was not working correctly on mac when zsh was being used. This fix resolves that issue.
Summary by CodeRabbit
Bug Fixes
Documentation