Add rvm and nvm support to ci-switch-config #1986
Conversation
Previously, the script only supported mise and asdf version managers. This adds support for rvm (Ruby) and nvm (Node.js) to make the script more flexible for developers using different version management tools. Key changes: - Detect rvm, nvm, or rvm+nvm combinations in addition to mise/asdf - Add set_ruby_version() helper to switch Ruby versions with rvm - Add set_node_version() helper to switch Node versions with nvm - Update both minimum and latest config functions to use new helpers - Add appropriate reload/verification instructions for each manager 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds multi-version-manager support to Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as ci-switch-config
participant Manager as Version Manager(s)
User ->> Script: run (switch-min / switch-latest / status / reload)
Script ->> Script: detect managers (mise / asdf / rvm / nvm / rvm+nvm)
alt rvm + nvm detected
Script ->> Manager: set_ruby_version() via RVM
Script ->> Manager: set_node_version() via NVM (attempt load from common locations)
else mise or asdf
Script ->> Manager: update `.tool-versions` (ruby, node)
else rvm only
Script ->> Manager: set_ruby_version() via RVM
else nvm only
Script ->> Manager: set_node_version() via NVM (no Ruby)
end
alt status / reload
Script ->> Manager: run manager-specific current/status commands
Note right of Manager: manager-aware guidance emitted\n(e.g., `mise current`, `asdf current`, `rvm current`, `nvm current`)
end
Script -->> User: report applied versions & shell-reload hints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Code Review for PR 1986: Add rvm and nvm support to ci-switch-configThanks for extending the ci-switch-config script to support rvm and nvm! This is a valuable addition that will help developers using different version managers. Strengths
Potential Issues1. Critical: nvm sourcing path assumptions (Lines 87-89)Lines 87-89 assume nvm is installed in HOME/.nvm, but many users install it via Homebrew (/opt/homebrew/opt/nvm) or in custom locations. Recommendation: Add fallback paths for common nvm installation locations (standard, Homebrew, XDG config). 2. Edge case: rvm version format mismatch (Line 67)Line 67 checks for ruby-VERSION but rvm might list versions differently depending on installation method. Recommendation: Make the grep pattern more flexible to handle versions with or without the ruby- prefix. 3. Missing error handling for rvm/nvm commands (Lines 71, 95)The rvm use and nvm use commands could fail if the version was just installed but something went wrong, or the user's shell isn't properly configured. Recommendation: Add error checking with helpful messages guiding users to source their shell configuration. 4. nvm version check inconsistency (Line 91)nvm list with a version argument doesn't always work as expected across different nvm versions. Recommendation: Check if the version exists in nvm list output instead. Documentation and Usability5. Missing documentation updateThe SWITCHING_CI_CONFIGS.md file (lines 55-72) still only mentions mise and asdf in Prerequisites. Should include rvm/nvm installation instructions. 6. User guidance for only nvm or only rvm casesThe script detects rvm or nvm alone, but rvm alone can't manage Node and nvm alone can't manage Ruby. Issue: Script will fail silently or with unclear errors if someone only has one version manager. Recommendation: Add validation in set_ruby_version and set_node_version to fail early with clear error messages like: Cannot set Ruby version: nvm doesn't manage Ruby. Please install rvm or use mise/asdf instead. Performance and Best Practices7. Shell sourcing won't persist (Lines 221-222)Lines 221-222 attempt to source asdf but this won't affect the user's shell after script exits. Recommendation: Add comment clarifying this only affects the script's subshell. Security ConsiderationsNo security concerns identified. The script:
Testing RecommendationsConsider manual testing scenarios:
Minor Suggestions
SummaryThis is a solid enhancement. Main concerns:
The code follows bash best practices and maintains consistency with existing script structure. Verdict: Approve with RecommendationsThe PR is functionally sound and valuable. Recommended changes would make it more robust for edge cases, but core functionality works well. Review generated by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
bin/ci-switch-config (2)
323-360: Code duplication is acceptable here.The reload and instruction logic is duplicated between switch_to_minimum and restore_to_latest. While this creates some duplication, it keeps each function self-contained and readable, which is often preferable in shell scripts.
If you want to reduce duplication, you could extract shared reload and instruction logic into helper functions, but this is not necessary:
reload_version_manager() { local version_manager="$1" case "$version_manager" in mise) # mise will auto-detect .tool-versions on next cd ;; asdf) if [ -f "$HOME/.asdf/asdf.sh" ]; then source "$HOME/.asdf/asdf.sh" fi ;; rvm|rvm+nvm) # Versions already set via rvm use ;; nvm) # Version already set via nvm use ;; esac } show_verification_commands() { local version_manager="$1" case "$version_manager" in mise) echo " mise current" ;; asdf) echo " asdf current" ;; rvm+nvm) echo " rvm current && nvm current" ;; rvm) echo " rvm current" ;; nvm) echo " nvm current" ;; esac }
57-98: Consider adding version verification after switching.The helper functions set versions but don't verify that the switch was successful. This could help catch issues early, especially given the complexities of nvm/rvm setup.
Add verification to each helper function:
set_ruby_version() { local version="$1" local version_manager="$2" case "$version_manager" in mise|asdf) # Handled via .tool-versions ;; rvm|rvm+nvm) print_header "Setting Ruby $version with rvm" if ! rvm list strings | grep -q "^ruby-${version}$"; then print_warning "Ruby $version not installed. Installing..." rvm install "$version" fi rvm use "$version" + # Verify the switch was successful + if ! ruby --version | grep -q "$version"; then + print_warning "Ruby version switch may not have worked. Current: $(ruby --version)" + fi ;; esac } set_node_version() { local version="$1" local version_manager="$2" case "$version_manager" in mise|asdf) # Handled via .tool-versions ;; nvm|rvm+nvm) print_header "Setting Node $version with nvm" # Source nvm if not already loaded if [ -z "${NVM_DIR:-}" ]; then export NVM_DIR="$HOME/.nvm" [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" fi if ! nvm list "$version" &> /dev/null; then print_warning "Node $version not installed. Installing..." nvm install "$version" fi nvm use "$version" + # Verify the switch was successful + if ! node --version | grep -q "v$version"; then + print_warning "Node version switch may not have worked. Current: $(node --version)" + fi ;; esac }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/ci-switch-config(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (4)
bin/ci-switch-config (4)
174-184: Good separation of concerns.The conditional logic correctly handles .tool-versions creation only for mise/asdf, while delegating version-specific setup to the helper functions. This maintains clean separation between version managers.
216-231: LGTM!The case statement correctly handles reload behavior for each version manager. The comments accurately note that rvm/nvm don't need reloading since
rvm useandnvm useset versions immediately, unlike mise/asdf which require shell reload or directory change.
237-253: Excellent user guidance.The manager-specific verification commands provide clear next steps for users. The
rvm current && nvm currentcombination for mixed setups is particularly helpful.
281-291: Consistent implementation.The restore_to_latest function follows the same pattern as switch_to_minimum with appropriate version numbers for the "latest" configuration. The implementation is consistent and correct.
Enhanced the ci-switch-config script based on code review feedback: **Error Handling & Validation:** - Add comprehensive error checking for rvm/nvm commands with helpful messages - Detect and fail early when only rvm or only nvm is installed - Guide users to source shell configs or install missing managers - Make rvm version check more flexible (handles with/without ruby- prefix) **nvm Installation Detection:** - Support multiple common nvm locations (standard, Homebrew, XDG config) - Try ~/.nvm, /opt/homebrew/opt/nvm, and ~/.config/nvm - Provide clear error messages showing which locations were tried **Documentation:** - Update SWITCHING_CI_CONFIGS.md with complete rvm+nvm installation guide - Add troubleshooting sections for each version manager - Include verification commands for all supported managers - Add note clarifying asdf sourcing only affects script's subshell 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #1986: Add rvm and nvm support to ci-switch-configOverall AssessmentStrong implementation that successfully extends version manager support. The code is well-structured, follows the existing patterns, and includes good error handling. A few suggestions for improvement below. ✅ Strengths
🔍 Code Quality Issues1. Potential nvm version check bug (bin/ci-switch-config:128)Issue: Recommendation: Use if ! nvm version "$version" &> /dev/null; then2. Sourcing nvm affects only script subshell (bin/ci-switch-config:106-124)The script sources nvm in its own process, but since it runs in a subshell, this doesn't persist to the user's shell. Consider adding clarification to the warning message. 3. rvm version check could be more robust (bin/ci-switch-config:68)Consider using 🚀 Performance Considerations
🔒 Security ConcernsNone identified. The script:
🧪 Test CoverageFrom the PR description, the test plan covers:
Additional testing recommendations:
📋 SuggestionsMinor improvements:
✨ Best Practices Adherence✅ Follows CLAUDE.md requirements:
VerdictApprove with minor suggestions - This is production-ready code that significantly improves the script's flexibility. The issues mentioned are minor and optional improvements. The current implementation is solid and will work well for the vast majority of users. Great work on maintaining backward compatibility while extending functionality! 🎉 🤖 Generated with Claude Code |
Based on additional code review feedback: **Version Detection Improvements:** - Use `rvm list rubies` instead of `rvm list strings` for more consistent output across different rvm versions - Use `nvm version VERSION` instead of `nvm list | grep` for more reliable version checking across different nvm versions **User Experience:** - Add explicit warning that rvm/nvm version changes only affect the script's subshell - Inform users they may need to open a new terminal or source their shell config for changes to take effect - Clarify the persistent vs. temporary nature of version switches These changes make the script more robust across different version manager installations and set clearer expectations for users. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code ReviewI've reviewed PR #1986 and here's my detailed feedback: Overall Assessment ✅This is a well-structured enhancement that successfully adds rvm/nvm support to the CI configuration switching script. The implementation is thoughtful, maintainable, and includes good error handling and user guidance. Strengths 👍1. Excellent Code Organization
2. Robust Error Handling
3. User Experience
4. Backward Compatibility
Issues & Recommendations 🔍1. Potential nvm Sourcing Issue
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/ci-switch-config (1)
42-47: Fix nvm detection to work with shell function installations.
command -v nvmfails to detect nvm in most installations because nvm is typically loaded as a shell function rather than an executable. This causes the script to incorrectly report "No version manager found" even when nvm is properly installed, breaking bothrvm+nvmand standalonenvmdetection.Apply this diff to use a detection method that works for both executables and functions:
elif command -v asdf &> /dev/null; then echo "asdf" - elif command -v rvm &> /dev/null && command -v nvm &> /dev/null; then + elif command -v rvm &> /dev/null && type nvm &> /dev/null; then echo "rvm+nvm" elif command -v rvm &> /dev/null; then echo "rvm" - elif command -v nvm &> /dev/null; then + elif type nvm &> /dev/null; then echo "nvm" elseThe
typebuiltin detects both commands and shell functions, making it reliable for nvm detection.
🧹 Nitpick comments (1)
bin/ci-switch-config (1)
106-106: Usetypeinstead ofcommand -vto properly detect nvm.
command -v nvmonly detects executables, not shell functions. If nvm is already loaded as a function (the common case), this check will incorrectly trigger the sourcing fallback, potentially causing duplicate sourcing or warnings.Apply this diff:
- if ! command -v nvm &> /dev/null; then + if ! type nvm &> /dev/null; thenThe
typebuiltin detects both commands and shell functions, ensuring we only source nvm.sh when nvm is genuinely unavailable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/ci-switch-config(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
🔇 Additional comments (3)
bin/ci-switch-config (3)
270-286: Well-structured reload logic for multiple version managers.The conditional reload logic appropriately handles each version manager's requirements. For mise, it relies on auto-detection; for asdf, it sources the init script (with appropriate comment about subshell limitations); and for rvm/nvm, it correctly notes that versions are already active via the
usecommands.Also applies to: 386-402
291-316: Excellent user guidance for version verification.The manager-specific warnings and verification commands provide clear, actionable guidance. The subshell warning for rvm/nvm users is particularly important, and the per-manager verification commands (
mise current,asdf current,rvm current && nvm current, etc.) help users confirm their environment is correctly configured.Also applies to: 407-432
84-89: Helpful error messages with actionable alternatives.The error messages when a manager can't handle a runtime (nvm for Ruby, rvm for Node) provide clear explanations and suggest appropriate alternatives with installation links. This guides users toward complete solutions like mise or asdf that handle both runtimes.
Also applies to: 144-149
Based on final code review feedback: **Version File Management:** - Create .ruby-version for rvm users (enables auto-switching on cd) - Create .nvmrc for nvm users (enables auto-switching on cd) - Maintains .tool-versions for mise/asdf users - Allows users to leverage each manager's native auto-switching features **Improved Version Matching:** - Fix rvm version check to avoid partial matches (3.2.8 won't match 3.2.80) - Use word boundaries in grep pattern for exact version matching **Enhanced User Warnings:** - Make subshell limitation warnings more explicit and prominent - Explain WHY versions don't persist (subshell vs. parent shell) - Provide clear, actionable steps with two distinct options: 1. Open new terminal (recommended) 2. Source shell config files (with exact commands) - Show appropriate sourcing commands based on version manager **Documentation:** - Add warning about not mixing version managers (mise + rvm, etc.) - Explain version manager priority (mise > asdf > rvm+nvm) - Clarify potential confusion from using multiple managers These changes make the script more user-friendly by enabling native auto-switching features and setting much clearer expectations about how version changes work with rvm/nvm. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fix critical regex bug where version detection could match unintended
versions. For example, searching for "3.2.8" could incorrectly match
"13.2.8" because the pattern wasn't anchored to the start of the line.
Changes:
- Add line-start anchor (^) to prevent matching partial strings
- Allow optional leading whitespace: ^[[:space:]]*
- Keep optional ruby- prefix: (ruby-)?
- Remove end anchor to allow patch suffixes (3.2.8-p123)
New pattern: ^[[:space:]]*(ruby-)?${version}
This ensures exact version matching at the start of each line while
still being flexible enough to match versions with or without the
ruby- prefix and with any patch suffix.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review: PR #1986 - Add rvm and nvm support to ci-switch-configOverall AssessmentThis PR successfully extends the Verdict: ✅ APPROVED - The code is production-ready with comprehensive improvements across 5 commits that addressed previous review feedback. 🎯 Strengths1. Excellent Error Handling and User Guidance
2. Robust nvm Detection and SourcingThe script tries multiple common nvm installation locations (lines 108-126):
This is much more robust than the typical approach and will work across different installation methods. 3. Smart Version Manager PriorityThe detection order is sensible (lines 38-54):
4. Progressive Improvement Through CommitsLooking at the commit history, the PR evolved nicely:
Each commit addressed specific concerns from reviews, showing good responsiveness to feedback. 5. Comprehensive DocumentationThe
🔍 Code Quality AnalysisVersion Manager Detection (lines 37-55)check_version_manager() {
if command -v mise &> /dev/null; then
echo "mise"
elif command -v asdf &> /dev/null; then
echo "asdf"
elif command -v rvm &> /dev/null && command -v nvm &> /dev/null; then
echo "rvm+nvm"
# ...
}Observation: Uses Impact: Low - The script handles this gracefully by attempting to source nvm from multiple locations in Ruby Version Setting (lines 57-94)# Anchor to start of line to avoid false matches (e.g., 3.2.8 shouldn't match 13.2.8)
# Allow optional leading whitespace and ruby- prefix, no end anchor to allow patch suffixes
if ! rvm list rubies | grep -qE "^[[:space:]]*(ruby-)?${version}"; thenExcellent: The regex pattern has been carefully crafted (commit 5d3ef2f) to:
This addresses the concerns from previous CodeRabbit reviews perfectly. Node Version Setting (lines 96-154)The nvm sourcing logic (lines 108-126) is particularly well done:
Good practice: Error handling for both installation and usage (lines 132-142) with helpful messages. File Management (lines 239-252, 377-390)The script now creates case "$VERSION_MANAGER" in
rvm|rvm+nvm)
print_header "Creating .ruby-version for rvm"
echo "3.2.8" > "$PROJECT_ROOT/.ruby-version"
print_success "Created .ruby-version with Ruby 3.2.8"
;;
esacExcellent addition: This allows rvm/nvm to automatically pick up versions when cd'ing into the directory, improving the user experience significantly. This was added in commit 06b7927. Shell Persistence Warnings (lines 308-320, 446-458)The warnings about version changes not persisting are clear and provide multiple options:
Well executed: Conditional display of sourcing commands based on which manager(s) are in use. 🛡️ Security & Best Practices✅ Good Practices Observed:
✅ Shell Safety:
📝 Documentation QualityThe Strong points:
Particularly helpful:
🔬 Edge Cases HandledThe implementation handles several edge cases well:
🎨 Code Style & MaintainabilityConsistency: The code follows consistent patterns:
Readability:
Maintainability:
🐛 Minor Observations (Not Blocking)1. nvm detection in check_version_manager()Lines 42, 46 use If you wanted to improve this, you could use
2. Potential for helper function extractionThe reload logic (lines 286-303, 424-441) and verification command display (lines 322-340, 460-478) are duplicated between Not a blocker: In bash scripts, sometimes duplication is preferable to complex abstractions, especially for clarity. The current approach is fine. ✅ Previous Review Feedback AddressedLooking at the CodeRabbit reviews in the PR comments, the key issues raised were:
All major concerns from previous reviews have been thoroughly addressed. 📊 Test CoverageFrom the PR description:
Recommendation for manual testing (if not already done):
🎯 Final VerdictStatus: ✅ APPROVED This PR is production-ready and well-implemented. The code:
The progressive improvements across 5 commits show excellent responsiveness to feedback and attention to detail. The script will significantly improve the experience for developers using rvm/nvm. No blocking issues identified. The minor observations mentioned above are optional enhancements, not requirements. Great work on this enhancement! 🎉 |
Summary
bin/ci-switch-configscriptKey Changes
check_version_manager()to detect rvm, nvm, or rvm+nvm combinationsset_ruby_version()helper function to switch Ruby versions with rvmset_node_version()helper function to switch Node versions with nvmswitch_to_minimum()to use new helper functionsrestore_to_latest()to use new helper functionsHow It Works
The script now detects version managers in this priority order:
For rvm/nvm users:
rvm useandnvm useto switch versionsTest Plan
bash -n bin/ci-switch-config)🤖 Generated with Claude Code
This change is
Summary by CodeRabbit