fix(cli): fail fast on unsupported onboarding hosts#1461
fix(cli): fail fast on unsupported onboarding hosts#1461ChunkyMonkey11 wants to merge 11 commits intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a host support detection library (Linux/macOS), a CommonJS shim, unit and integration tests, and integrates the host support check into onboarding preflight to log results and exit early on unsupported hosts. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Onboard as Onboard (preflight)
participant HostSupport as HostSupport
participant OS as OS/FileSystem
participant Docker as DockerChecks
User->>Onboard: run onboard (preflight)
Onboard->>HostSupport: checkHostSupport(opts)
HostSupport->>OS: read /etc/os-release or run sw_vers
OS-->>HostSupport: return version info or error
HostSupport-->>Onboard: HostSupportResult (ok|warning|error)
alt status == "error"
Onboard->>Onboard: log to stderr
Onboard-->>User: exit(1)
else status == "ok" or "warning"
Onboard->>Onboard: log to stdout
Onboard->>Docker: run docker & other checks
Docker-->>Onboard: continue startup
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/host-support.ts (1)
143-151: Consider simplifying the EOL condition.The condition
major < 20 || (major === 18 && minor <= 4)is logically redundant —major < 20already covers all 18.x versions. The second clause(major === 18 && minor <= 4)is a subset ofmajor < 20and never evaluated independently.If the intent is to mark all pre-20.x as EOL,
major < 20alone suffices. If only specific 18.x versions should be EOL, the condition needs restructuring.Simplified condition (if all pre-20.x is intended as EOL)
- if (major < 20 || (major === 18 && minor <= 4)) { + if (major < 20) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/host-support.ts` around lines 143 - 151, The EOL check uses a redundant clause: simplify the if-condition to remove the never-reached subset and make intent explicit; replace the current `if (major < 20 || (major === 18 && minor <= 4))` logic with either a single `major < 20` when all pre-20.x releases are EOL or, if only specific 18.x minors should be EOL, change it to a focused check like `major === 18 && minor <= 4`; update the branch that returns the object containing os, version (normalized), status, code, and message accordingly so the condition accurately reflects the intended EOL policy for variables major, minor, and normalized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/host-support.ts`:
- Around line 143-151: The EOL check uses a redundant clause: simplify the
if-condition to remove the never-reached subset and make intent explicit;
replace the current `if (major < 20 || (major === 18 && minor <= 4))` logic with
either a single `major < 20` when all pre-20.x releases are EOL or, if only
specific 18.x minors should be EOL, change it to a focused check like `major ===
18 && minor <= 4`; update the branch that returns the object containing os,
version (normalized), status, code, and message accordingly so the condition
accurately reflects the intended EOL policy for variables major, minor, and
normalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98c1c43d-3aa0-418e-bfd0-40775b730070
📒 Files selected for processing (5)
bin/lib/host-support.jsbin/lib/onboard.jssrc/lib/host-support.test.tssrc/lib/host-support.tstest/onboard.test.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✨ Thanks for submitting this pull request, which proposes a way to fail fast during onboarding when the host OS is unsupported, improving the setup experience. |
|
Hi @wscurran, quick update: I merged the latest upstream/main into this branch and resolved the onboarding preflight conflict. The branch is now up to date and ready for commit/review on your side. |
Summary
error.warninghosts while surfacing clear guidance.Root Cause
Onboarding did not gate early on unsupported host OS/platform combinations, which could lead users into later-stage failures after expensive setup steps.
Changes
src/lib/host-support.tswith Linux/macOS/platform classification helpers.bin/lib/host-support.js.checkHostSupport()intobin/lib/onboard.jspreflight.src/lib/host-support.test.ts.test/onboard.test.jsto verify fail-fast behavior before Docker checks.src/lib/host-support.tsfrom a redundant clause tomajor < 20.Validation
npm test -- src/lib/host-support.test.ts test/onboard.test.jsSigned-off-by: Revant Patel revant.h.patel@gmail.com
Summary by CodeRabbit
New Features
Tests