Skip to content

Conversation

@ZanCorDX
Copy link
Collaborator

@ZanCorDX ZanCorDX commented Feb 2, 2026

πŸ“ Summary

  • ARM64 docker
  • Speed improvement by recycling built bin.

βœ… I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@claude
Copy link
Contributor

claude bot commented Feb 2, 2026

Claude finished @ZanCorDX's task in 1m 15s β€”β€” View job


Review Summary

I've reviewed this PR which adds ARM64 Docker support and optimizes build speed by reusing pre-built binaries.

Issues Found

1. Inconsistent base image versions (docker/Dockerfile.rbuilder-from-binary:5)

  • New Dockerfile uses cc-debian13 while existing Dockerfile.rbuilder uses cc-debian12. This inconsistency could lead to unexpected runtime behavior differences.

2. ARM64 build doesn't benefit from speed optimization (.github/workflows/release.yaml:389)

  • The build-docker-linux-arm64 job always builds from source, unlike the AMD64 job which can reuse pre-built binaries. The speed improvement from "recycling built bin" only applies to AMD64.
  • To achieve the stated speed improvement for ARM64, you'd need to add an ARM64 linux target to the build-binary matrix and download it in the ARM64 docker job.

3. Minor: No timeout on manifest creation step (.github/workflows/release.yaml:423)

  • The manifest creation script could hang indefinitely if Docker registry operations stall.

Additional Observations

  • The fallback logic in build-docker-linux-amd64 (lines 284-300) is well designed - it gracefully falls back to building from source if binary artifact isn't available.
  • The multi-arch manifest creation properly validates that both platform images exist before creating the manifest.
  • The new Dockerfile.rbuilder-from-binary correctly uses --chmod=755 to restore execute permissions lost during artifact upload.

Tasks

  • Gather context and read changed files
  • Analyze workflow changes in .github/workflows/release.yaml
  • Analyze new Dockerfile docker/Dockerfile.rbuilder-from-binary
  • Review for bugs, security, and best practices
  • Post review findings

# The CI job downloads the linux/amd64 rbuilder binary into the build context as ./rbuilder
# Artifact download does not preserve execute bit; --chmod sets it on copy.

FROM gcr.io/distroless/cc-debian13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing Dockerfile.rbuilder uses cc-debian12 but this uses cc-debian13. Consider keeping consistency between Dockerfiles:

Suggested change
FROM gcr.io/distroless/cc-debian13
FROM gcr.io/distroless/cc-debian12

If debian13 is intentional (e.g., for newer glibc compatibility), consider updating Dockerfile.rbuilder as well to maintain consistency.

username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: docker build and push (from source, native arm64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ARM64 build does not attempt to download and reuse a pre-built binary like the AMD64 job does. This means ARM64 always builds from source, missing the speed improvement mentioned in the PR description.

Consider adding a build-binary job for aarch64-unknown-linux-gnu target, then download and use it here similar to the AMD64 job.

username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: Create and push multi-arch manifest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a timeout-minutes to the script to prevent indefinite hanging if imagetools inspect or imagetools create stalls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant