Reject unsupported host platforms in the container build fallback - #239
Conversation
…lback #238 review suggestion: process.platform === 'darwin' ? 'darwin' : 'linux' silently handed a win32 host a linux binary, reproducing the bug class the cross-compile fixed. Unsupported platforms now throw with the --rpk-bin escape hatch named.
✅ Deploy Preview for docs-extensions-and-macros ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe container-based fallback build now checks the host platform before cross-compilation. macOS and Linux hosts continue through the existing path. Other hosts now receive an explicit error instead of being mapped to Linux. The error instructs users to update Go or provide an existing binary. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/rpk-docs/rpk-docs-handler.js`:
- Around line 1389-1398: Move the host-platform validation in the `catch` block
to its beginning, before the Docker availability check and related fallback
handling. Ensure unsupported platforms always throw the platform-specific error
containing the `--rpk-bin` escape hatch, while supported macOS and Linux hosts
retain the existing Docker error behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 62fd974d-ba6a-4286-81af-a0700d133190
📒 Files selected for processing (1)
tools/rpk-docs/rpk-docs-handler.js
| // Only macOS and Linux hosts are supported: mapping anything else to | ||
| // linux would hand a Windows host an unrunnable binary, the exact bug | ||
| // class the cross-compile fixes (review finding on #238). | ||
| if (process.platform !== 'darwin' && process.platform !== 'linux') { | ||
| throw new Error( | ||
| `${nativeErr.message}\n` + | ||
| `Container fallback does not support host platform "${process.platform}". ` + | ||
| 'Update Go, or pass --rpk-bin <path> to use an existing rpk binary.' | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the host-platform guard before the Docker check.
Line [1392] runs after the Docker availability check. If an unsupported host has no available Docker daemon, the function throws the earlier Docker error and does not identify the unsupported platform. Move this guard to the start of the catch block. This ensures that unsupported hosts always receive the platform-specific error and the --rpk-bin escape hatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/rpk-docs/rpk-docs-handler.js` around lines 1389 - 1398, Move the
host-platform validation in the `catch` block to its beginning, before the
Docker availability check and related fallback handling. Ensure unsupported
platforms always throw the platform-specific error containing the `--rpk-bin`
escape hatch, while supported macOS and Linux hosts retain the existing Docker
error behavior.
micheleRP
left a comment
micheleRP
left a comment
There was a problem hiding this comment.
Approving. This is exactly suggestion 3 from the #238 review, implemented minimally: unsupported hosts throw instead of being mapped to linux, the original nativeErr message is preserved so the Go-version cause is not lost, and the error names --rpk-bin as the way forward. macOS and Linux paths are untouched.
Two nits, neither blocking:
- The check sits after
console.warn('...building in a container...'), so a Windows user is told a container build is starting and then told the platform is unsupported. Moving the guard above that warn would avoid announcing work that never happens. - No test. Stubbing
process.platformis awkward enough that I would not insist, and the code is a three-line guard, so this is optional.
Sequencing: since this has no version bump, it needs to merge before #228 so the 5.3.1 publish carries it.
2 participants
Suggestion 3 from @micheleRP's #238 review: the host-platform mapping sent
win32tolinux, silently reproducing the wrong-binary bug class the cross-compile fix eliminated. Unsupported platforms now throw with a message naming--rpk-binas the escape hatch. macOS and Linux behavior unchanged.No version bump — intended to merge before #228 so the 5.3.1 publish carries it with the rest of the train.
Related PRs (rpk docs automation train)
Follow-up to #238.