You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review complete. Three issues flagged inline; summary below.
Read PR diff
Analyze playground/releases.go changes
Analyze playground/recipe_yaml.go changes
Check tests coverage
Review custom recipe/doc updates
Post review
Findings
playground.yaml:13-14 — commented-out #version: "v1.11.3" left next to active v2.0.0 is dead config without context; either drop it or add a one-line rationale.
releases.go:70-79 — the new binary-arch branch duplicates the binary branch almost verbatim; worth collapsing. Also raised a pre-existing partial-download/cache-poisoning issue in downloadBinary that binary-arch inherits (failed mid-transfer leaves a broken file that the next run treats as cached).
recipe_yaml.go:654 — the default Arch mapping hardcodes x86_64-unknown-linux-gnu on any Linux host. Pre-existing, but binary-arch encodes arch in the asset path, so linux/arm64 hosts will now fetch the amd64 binary (or 404) instead of aarch64-unknown-linux-gnu, which rbuilder v1.3.15 does publish.
Tests for the new binary-arch path (happy path + unsupported-arch error) look good and use an httptest server consistently with existing tests.
• Branch
playground.yaml commented-out version — ✅ addressed by 46125c9. Current line version: "v1.11.3" # v2.0.0 not supported yet gives the rationale inline; no longer dead config.
releases.gobinary / binary-arch branch duplication + partial-download cache poisoning — ❗ still open. See prior inline comment. The binary-arch branch at releases.go:70-79 still duplicates the binary branch's download-and-chmod logic, and downloadBinary still writes directly to outPath before io.Copy completes — a mid-transfer failure leaves a partial file that the cache check at releases.go:37-43 treats as valid on the next run.
recipe_yaml.go Linux arch mapping hardcoded to x86_64 — ❗ still open. See prior inline comment. With binary-arch now encoding arch in the URL, a linux/arm64 host (ARM cloud VMs, Apple Silicon containers) will request rbuilder-v1.3.15-x86_64-unknown-linux-gnu and either 404 or pull an amd64 binary it can't execute. rbuilder v1.3.15 publishes aarch64-unknown-linux-gnu — worth mapping.
The new commits (d1b3858, 46125c9) only touch custom-recipes/rbuilder/bin/playground.yaml and don't introduce new issues.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adapts builder-playground to support the new arch and OS based binary naming pattern for rbuilder releases.