Host-platform container builds and unsatisfiable env-gating validation - #238
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
✅ Deploy Preview for docs-extensions-and-macros ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…and reject unsatisfiable env gating Two review follow-ups from the 5.3.0 train (non-blocking findings by micheleRP): - The Go-too-old container fallback in acquireRpkBinary built with the container's implicit GOOS=linux, then the darwin host tried to execute the linux binary and plugin installs died with no output. The build now cross-compiles for the host platform (rpk is CGO-free) by passing GOOS/GOARCH/CGO_ENABLED into the container. CI behavior is unchanged: linux host builds a linux binary as before. - Setting cloudOnly and selfHostedOnly on the same command or flag emits an ifdef::env-cloud + ifndef::env-cloud pair no build can satisfy, so the row silently vanished from BOTH sites with no warning. validate-overrides now reports it as an error at command and flag level.
…y path micheleRP's follow-up on redpanda-data/docs#1844: the plugin-release receiver excludes ai from --update-whats-new because rpk ai's docs home is adp-docs and the ADP release notes cover its CLI changes, but the full-regeneration path still wrote the whole tree into the block. A full run against the v26.2.1-rc2 snapshot produced a What's new dominated by rpk ai entries (the llm to llm-provider and mcp to mcp-server renames alone add 21 new plus 21 removed bullets). The generator now owns the rule instead of the workflows: filterDiffForWhatsNew drops excluded subtrees (rpk ai) from every category before the block renders, on both the from-source and from-json diff paths, and a --plugin ai refresh skips the What's new write with a log line. Diff reports and PR summaries keep the full picture. Verified against the real rc2 snapshot: zero rpk ai mentions in the block, 42 Self-Managed bullets remain, rpk check and rpk k8s entries intact.
2556685 to
97a9063
Compare
micheleRP
left a comment
There was a problem hiding this comment.
Both findings are correctly fixed and I verified each by running it rather than reading the diff. Approving, with two coordination notes.
Verification
| Fix | Result |
|---|---|
| Container fallback | The exact scenario that failed on my machine yesterday now succeeds: Native build failed (Go version mismatch: installed 1.24.4, required >= 1.26.4); building in a container... then Installing plugin: rpk ai install, Extracted flags for 52 command(s), Spliced fresh 'ai' subtree (version 0.2.32), exit 0 |
GOARCH mapping |
Correct, and easy to get wrong: process.arch === 'arm64' ? 'arm64' : 'amd64' maps x64 to amd64 instead of passing Node's name straight through |
| Mutual exclusion | Both levels caught, with precise contexts: commands["rpk cluster health"] and commands["rpk topic analyze"].flags.batches |
| What's-new filter | Zero rpk ai entries in the published block, block down from 113 lines to 59, and the now-empty Removed commands section is dropped rather than rendered empty |
| Filter completeness | Covers all 10 detail keys report-delta.js actually produces, so nothing under-filters |
| Reviewer visibility | The PR summary still shows the full picture (48 new and 21 removed commands). Only the published page is filtered, which matches the comment's stated intent |
| Tests | 904 passing, up from 862. CI 16 green, 0 failures |
Suggestions
-
The body describes two changes but this ships three. The What's-new exclusion is the most user-visible change here, since it alters what reaches the Self-Managed release notes, and a reviewer going by the body would not know to look for it. Worth describing so the behavior change is on the record.
-
Merge sequencing against #228. With #225, #226, and #229 merged, main is at 5.3.0. This PR has no bump and rides #228's 5.3.1, so if #228 merges first these fixes sit in main unpublished until some later bump. Either merge this before #228 or give it its own bump.
-
A Windows host silently gets a Linux binary.
process.platform === 'darwin' ? 'darwin' : 'linux'mapswin32tolinux, which reproduces the original bug class. Minor, since the toolchain is macOS and Linux, but throwing for unsupported platforms would be clearer than a wrong default. -
Contradictory overrides are reported but still ship. Validation errors are non-fatal, so that run still emits the unsatisfiable row and it vanishes from both builds, which is the thing the error warns about. Consistent with the existing design, and #225's Override Validation section now surfaces it in the generated PR body, so this is probably fine as-is. Flagging only so the choice is deliberate.
Adjacent, not this PR
rpk container statusrenders two== Exampleheadings (lines 32 and 37 of the generated page). Its override contributes a curated Example section atposition: endwhile the generator emits one as well. Same collision class asrpk connect run's Flags, but the curated-section-wins logic from #225 is scoped to titles matchingFlags, so Example only trips the duplicate-heading warning. Either arpk-overrides.jsonfix in the docs repo or a generalization of that rule, whichever you prefer.- The run still reports the 4 stale override paths. redpanda-data/docs#1865 removes those.
What works well
- The cross-compile fix is the right shape, and
CGO_ENABLED=0with the note that rpk is CGO-free is the detail that makes it safe. - Linux CI is genuinely unaffected: a Linux host still resolves to
GOOS=linux, so it is a no-op there. - The plugin path also skips What's-new for excluded subtrees, so the two entry points now agree instead of contradicting each other.
filterDiffForWhatsNewreturns a copy, and the test asserts the input is untouched, so diff reports and PR summaries keep full fidelity while only the published block is filtered.- The
rpk aimcase in the tests is exactly the edge a prefix filter gets wrong.
|
Thanks for the run-it-first verification — all four suggestions actioned or answered:
Adjacent items:
|
Three non-blocking review findings from the 5.3.0 train review by @micheleRP, fixed with tests:
macOS container fallback built an unrunnable binary.
acquireRpkBinary's Go-too-old fallback built insidegolang:<ver>with the container's implicitGOOS=linux, and the darwin host then executed a linux binary — plugin installs died with no output. The container build now cross-compiles for the host platform (GOOS/GOARCHfromprocess.platform/process.arch,CGO_ENABLED=0— rpk is CGO-free). Linux CI is unaffected.--rpk-binremains the offline escape hatch.cloudOnly+selfHostedOnlyon the same command or flag silently deleted the row from every build. The pair emitsifdef::env-cloudwrapped inifndef::env-cloud, which no build satisfies.validate-overrides.jsnow reports it as an error at both command and flag level (feat(rpk-docs): gate subcommand table rows by cloud availability #229 review suggestion).rpk aino longer reaches the Self-Managed What's new on any path (follow-up on redpanda-data/docs#1844). The plugin-release receiver already excluded ai from--update-whats-newbecause rpk ai's docs home is adp-docs, but full regenerations still wrote the whole tree into the block — against the rc2 snapshot the result was dominated by rpk ai rename churn (21 new + 21 removed bullets). The generator now owns the rule:filterDiffForWhatsNewdrops therpk aisubtree from every category on both full-generation paths, and a--plugin airefresh skips the What's-new write with a log line. Diff reports and PR summaries keep the full picture. Verified against the real v26.2.1-rc2 snapshot: zerorpk aimentions, 42 Self-Managed bullets remain,rpk check/rpk k8sintact.Full suite 904/904 green. No version bump: merged before #228 so its 5.3.1 publish carries all three, in place before the v26.2.1 re-dispatch (the run that would have carried the polluted What's-new block).
Related PRs (rpk docs automation train)
Follow-ups to #225, #229, and redpanda-data/docs#1844.