-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat: move to use deb822 .sources #1911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded
⌛ 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)
WalkthroughDebian install scripts now create structured APT Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/deb/script_generator/base_script.sh (1)
92-109: Gate N|Solid pinning/messaging to LTS-only versions.The DEB base template unconditionally configures N|Solid preferences and messaging for every generated version script. This contradicts the LTS-only policy and creates inconsistency with RPM scripts, which correctly gate N|Solid to LTS releases (20.x, 22.x, 24.x). Non-LTS versions like 25.x should not advertise N|Solid availability. Add a version guard in the template to emit N|Solid configuration and messaging only for LTS branches.
♻️ Proposed fix (update LTS list as versions change)
- # N|solid Config - echo "Package: nsolid" | tee /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nsolid > /dev/null + local enable_nsolid=false + case "$NODE_VERSION" in + 20.x|22.x|24.x) enable_nsolid=true ;; # update as LTS set changes + esac + if $enable_nsolid; then + # N|solid Config + echo "Package: nsolid" | tee /etc/apt/preferences.d/nsolid > /dev/null + echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nsolid > /dev/null + echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nsolid > /dev/null + fi ... - log "You can use N|solid Runtime as a node.js alternative" "info" - log "To install N|solid Runtime, run: apt install nsolid -y \n" "success" + if $enable_nsolid; then + log "You can use N|solid Runtime as a node.js alternative" "info" + log "To install N|solid Runtime, run: apt install nsolid -y \n" "success" + fiscripts/deb/setup_25.x (1)
92-109: Remove N|Solid pinning from 25.x setup.
Per prior repo learnings, N|Solid should only be configured for LTS releases (18.x/20.x/22.x). Keeping it here risks advertising an unsupported runtime. Please drop the N|Solid preferences and messaging in this non‑LTS script.Based on learnings, DEB scripts should only configure N|Solid for LTS versions.🔧 Suggested change
- # N|solid Config - echo "Package: nsolid" | tee /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nsolid > /dev/null - # Nodejs Config echo "Package: nodejs" | tee /etc/apt/preferences.d/nodejs > /dev/null echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nodejs > /dev/null echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nodejs > /dev/null @@ - log "You can use N|solid Runtime as a node.js alternative" "info" - log "To install N|solid Runtime, run: apt install nsolid -y \n" "success" + # N|Solid is only supported on LTS lines; omit messaging herescripts/deb/setup_23.x (1)
92-109: Remove N|Solid pinning from 23.x setup.
N|Solid should only be configured for LTS lines (18.x/20.x/22.x). Keeping these prefs/messages in 23.x risks advertising an unsupported runtime.Based on learnings, DEB scripts should only configure N|Solid for LTS versions.🔧 Suggested change
- # N|solid Config - echo "Package: nsolid" | tee /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nsolid > /dev/null - echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nsolid > /dev/null - # Nodejs Config echo "Package: nodejs" | tee /etc/apt/preferences.d/nodejs > /dev/null echo "Pin: origin deb.nodesource.com" | tee -a /etc/apt/preferences.d/nodejs > /dev/null echo "Pin-Priority: 600" | tee -a /etc/apt/preferences.d/nodejs > /dev/null @@ - log "You can use N|solid Runtime as a node.js alternative" "info" - log "To install N|solid Runtime, run: apt install nsolid -y \n" "success" + # N|Solid is only supported on LTS lines; omit messaging here
🤖 Fix all issues with AI agents
In `@scripts/deb/setup_current.x`:
- Line 61: The DEB base_script currently configures N|Solid unconditionally;
update scripts/deb/script_generator/base_script.sh to wrap the N|Solid
repository/keys logic in a conditional that checks NODE_VERSION against LTS
releases only (e.g., match "18.x", "20.x", "22.x", "24.x"), using the same
pattern as rpm/script_generator/base_script.sh (check NODE_VERSION with ||
combined [[ ]] tests) so N|Solid is applied only for those LTS versions.
riosje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yaml:
- Around line 19-20: The workflow matrix includes Node.js 25 but NSolid is only
available for LTS versions 20/22/24; update every NSolid install/validation step
to run only when matrix.version is one of those LTS values by adding an if
condition that checks matrix.version == 20 || matrix.version == 22 ||
matrix.version == 24 (apply this gating to all NSolid-related steps referenced
in the file, e.g., any steps named like "Install NSolid" or "Validate NSolid" or
conditionally executing NSolid install commands that currently rely on
matrix.version).
dd48dab to
32b0b34
Compare
riosje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary by CodeRabbit
Chores
Chores
✏️ Tip: You can customize this high-level summary in your review settings.