Support ARM64 architecture when running docker tests - #35
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds architecture-aware Go installation to four Dockerfiles (Ubuntu, Alpine, AlmaLinux, Rocky) using build-time args (GO_VERSION/TARGETARCH) and validation, updates Docker test README with an "Architecture Support" subsection listing amd64 and arm64, and adjusts two docker-compose test commands to use Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Builder as Docker Build
participant Image as Base Image
participant RUN as Docker RUN step
Dev->>Builder: docker build --build-arg GO_VERSION=... --build-arg TARGETARCH=...
Builder->>Image: create image layers
Image->>RUN: execute install RUN
RUN->>RUN: check $TARGETARCH
alt TARGETARCH set (amd64/arm64)
RUN->>RUN: construct URL https://go.dev/dl/go${GO_VERSION}.linux-${TARGETARCH}.tar.gz
RUN->>RUN: download, extract to /usr/local, set PATH/GOROOT, cleanup
RUN-->>Builder: success -> continue build
else TARGETARCH unset or unsupported
RUN-->>Builder: echo error and exit 1
end
Builder-->>Dev: build finished (success/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
Summary of ChangesHello @brunodam, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the project's testing infrastructure by enabling Docker-based tests to run seamlessly on ARM64 architecture, in addition to the existing AMD64 support. By dynamically selecting the appropriate Go toolchain based on the host's architecture, it eliminates previous compatibility issues, thereby broadening test coverage and improving the development experience for a wider range of hardware platforms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![ellipsis-dev[bot]](https://cdn.github.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to bdbcc36 in 1 minute and 39 seconds. Click for details.
- Reviewed
99lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. testing/docker/README.md:43
- Draft comment:
Documentation now reflects ARM64 support; ensure this stays in sync with the Dockerfile implementations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. testing/docker/alpine.Dockerfile:13
- Draft comment:
The auto-detect logic is clear and consistent. Consider stricter shell error handling and parameterizing the Go version for future updates. - Reason this comment was not posted:
Marked as duplicate.
3. testing/docker/rockylinux.Dockerfile:13
- Draft comment:
Consistent auto-detection implementation; consider adding robust error flags (set -euxo pipefail) and parameterizing the Go version as a build argument. - Reason this comment was not posted:
Marked as duplicate.
4. testing/docker/ubuntu.Dockerfile:18
- Draft comment:
Using dpkg for architecture detection works well on Ubuntu. For consistency, consider parameterizing the Go version and using shell error flags for improved robustness. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is partially about the changes (architecture detection) but mostly suggests additional improvements. Parameterizing Go version is a reasonable suggestion but not strictly necessary. The shell error flags suggestion is vague - it's not clear what specific issue this would solve. The architecture detection part is actually working fine as implemented. The suggestions could improve code maintainability and robustness, and these are legitimate DevOps best practices. Maybe I'm being too harsh on useful improvements. While the suggestions are valid best practices, they're more like nice-to-have improvements rather than necessary fixes. The current implementation is functional and safe. The comment should be deleted as it's primarily suggesting optional improvements rather than addressing actual issues with the changes.
5. testing/docker/rockylinux.Dockerfile:13
- Draft comment:
Typographical note: There appears to be extra trailing whitespace after "auto-detect architecture". Please remove the trailing spaces. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Trailing whitespace in comments is a very minor stylistic issue. It doesn't affect functionality at all. In Dockerfiles specifically, trailing whitespace in comments has zero impact on the build or execution. This feels like the kind of nitpicky comment that adds noise without value. Maybe there's a strict style guide that requires no trailing whitespace anywhere? Maybe this could cause issues with some Docker tooling I'm not aware of? Even if there is a style guide, this is too minor to warrant a comment. Docker is very forgiving of whitespace in comments, and this won't cause any issues. This comment should be deleted as it's pointing out an extremely minor stylistic issue that has no functional impact.
Workflow ID: wflow_KqjJdYVc7zD4SpuB
You can customize 
![gemini-code-assist[bot]](https://cdn.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds ARM64 support to the Docker-based testing environment, which is a great improvement for developers on ARM systems. The approach of detecting the architecture in each Dockerfile works, but it introduces code duplication and hardcoded values. My review includes suggestions to refactor this by using Docker's built-in TARGETARCH argument and parameterizing the Go version. This will make the Dockerfiles more maintainable and aligned with best practices for multi-platform builds. Additionally, the README.md file needs a small correction to accurately list the distributions with new ARM64 support.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
testing/docker/rockylinux.Dockerfile (1)
13-20: Prefer Docker’s$TARGETARCH(and a sharedGO_VERSION) over invokinguname.BuildKit already exposes the target architecture via the implicit
TARGETARCHbuild arg, so we don’t need to shell out touname. UsingTARGETARCHalso handles aliases likearm64transparently and keeps the layer deterministic even when cross-building. While you’re here, promoting the Go version to anARGkeeps the value in one place instead of hard-coding it inside thecurlURL. The same refactor can be applied to the other Dockerfiles in this PR.+# Allow overriding Go version/architecture at build time +ARG GO_VERSION=1.23.4 +ARG TARGETARCH -# Install Go 1.23.4 - auto-detect architecture -RUN ARCH=$(uname -m) && \ - case $ARCH in \ - x86_64) GOARCH=amd64 ;; \ - aarch64) GOARCH=arm64 ;; \ - *) echo "Unsupported architecture: $ARCH" && exit 1 ;; \ - esac && \ - curl -L https://go.dev/dl/go1.23.4.linux-${GOARCH}.tar.gz | tar -C /usr/local -xz +# Install Go 1.23.4 - auto-detect architecture +RUN case "$TARGETARCH" in \ + amd64|arm64) GOARCH="$TARGETARCH" ;; \ + *) echo "Unsupported architecture: $TARGETARCH" && exit 1 ;; \ + esac && \ + curl -fL "https://go.dev/dl/go${GO_VERSION}.linux-${GOARCH}.tar.gz" | tar -C /usr/local -xz
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
testing/docker/README.md(1 hunks)testing/docker/almalinux.Dockerfile(1 hunks)testing/docker/alpine.Dockerfile(1 hunks)testing/docker/rockylinux.Dockerfile(1 hunks)testing/docker/ubuntu.Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: For multi-OS and package manager testing, use Docker-based workflows and configuration-driven matrices to ensure comprehensive coverage.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Tests use build tags (unit, integration, system, apt, yum, dnf, apk) for selective execution.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Testing strategy uses a three-layer approach: unit tests (parser, logic), integration tests (real command output), and system tests (privileged operations), with Docker-based multi-OS testing for reproducibility.
Applied to files:
testing/docker/README.md
🔇 Additional comments (4)
testing/docker/README.md (1)
43-46: Fedora Dockerfile installs Go via DNF, which is architecture-aware
It usesdnf install golang, so on ARM64 hosts it will pull the proper Go toolchain. The README’s “Full support” claim is accurate.testing/docker/almalinux.Dockerfile (1)
13-20: Go install logic now covers both amd64 and arm64.Mapping
uname -mto Go’s expectedGOARCHtokens and failing fast on unknown architectures is exactly what we needed to make these images multi-arch friendly. ✅testing/docker/ubuntu.Dockerfile (1)
17-24: Architecture-aware Go download looks correct.Using
dpkg --print-architecturekeeps the mapping native to Debian/Ubuntu derivatives, and the guard for unsupported arches will surface problems immediately.testing/docker/alpine.Dockerfile (1)
13-20: Alpine build now handles amd64/arm64 cleanly.The
uname -mmapping aligns with upstream Go tarball naming, so this will run out-of-the-box on Apple Silicon and other arm64 hosts.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testing/docker/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Tests use build tags (unit, integration, system, apt, yum, dnf, apk) for selective execution.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: For multi-OS and package manager testing, use Docker-based workflows and configuration-driven matrices to ensure comprehensive coverage.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Testing strategy uses a three-layer approach: unit tests (parser, logic), integration tests (real command output), and system tests (privileged operations), with Docker-based multi-OS testing for reproducibility.
Applied to files:
testing/docker/README.md
- Add automatic architecture detection in all Dockerfiles - Support both AMD64 (x86_64) and ARM64 (Apple Silicon) architectures - Use appropriate Go binary download based on detected architecture - Update documentation with architecture support details - Enable cross-platform Docker testing for developers using ARM64 machines Signed-off-by: Bruno De Assis Marques <bruno.marques@swirldslabs.com>
fd51755 to
6908f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
testing/docker/README.md (1)
43-44: Clarify ARM64 coverage by distroThe surrounding section still flags Fedora and Alpine as “implementation in progress,” but this new bullet now reads as though every listed OS is fully validated on both architectures. Please call out which test images are known-good on ARM64 versus still pending so ARM users don’t chase a setup that isn’t ready yet.
Consider revising as follows to match the current state:
**Architecture Support:** -- **AMD64 (x86_64), ARM64 (aarch64/Apple Silicon)** +- **AMD64 (x86_64)**: All distributions listed above +- **ARM64 (aarch64/Apple Silicon)**: Ubuntu 22.04, AlmaLinux 8, Rocky Linux 8 (Fedora & Alpine builds in progress)testing/docker/almalinux.Dockerfile (1)
14-21: Match the bash shell/pipefail patternTo keep failure handling uniform with the Ubuntu image (and the other Dockerfiles in this PR), it’d be good to switch this stage to the same
SHELL ["/bin/bash", "-euxo", "pipefail", "-c"]wrapper before thecurl | tarinvocation. That guarantees we bail out if the download stream dies mid-transfer.ARG GO_VERSION=1.23.4 ARG TARGETARCH +SHELL ["/bin/bash", "-euxo", "pipefail", "-c"] RUN if [ -z "${TARGETARCH}" ]; then \ echo "Error: TARGETARCH is not set. Use a BuildKit-enabled builder." >&2; \ exit 1; \testing/docker/docker-compose.test.yml (1)
51-75: Align fixture filenames with vim-enhancedNow that we query
vim-enhanced, consider renaming the captured fixture files to match the package name so future readers (and any string-matching tests) don’t assume the contents are still for thevimmeta package.- yum info vim-enhanced > testing/fixtures/yum/info-vim-rocky8.txt 2>/dev/null || true && + yum info vim-enhanced > testing/fixtures/yum/info-vim-enhanced-rocky8.txt 2>/dev/null || true && ... - yum info vim-enhanced > testing/fixtures/yum/info-vim-alma8.txt 2>/dev/null || true + yum info vim-enhanced > testing/fixtures/yum/info-vim-enhanced-alma8.txt 2>/dev/null || true(If you make this change, remember to update any consumers of those fixture names.)
testing/docker/rockylinux.Dockerfile (1)
13-21: Reuse the bash/pipefail shell here as wellSame rationale as AlmaLinux: adopting the
bash -euxo pipefailshell keeps the download/install step consistent across images and hard-fails on a broken stream.ARG GO_VERSION=1.23.4 ARG TARGETARCH +SHELL ["/bin/bash", "-euxo", "pipefail", "-c"] RUN if [ -z "${TARGETARCH}" ]; then \ echo "Error: TARGETARCH is not set. Use a BuildKit-enabled builder." >&2; \ exit 1; \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
testing/docker/README.md(1 hunks)testing/docker/almalinux.Dockerfile(2 hunks)testing/docker/alpine.Dockerfile(1 hunks)testing/docker/docker-compose.test.yml(2 hunks)testing/docker/rockylinux.Dockerfile(1 hunks)testing/docker/ubuntu.Dockerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/docker/alpine.Dockerfile
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Tests use build tags (unit, integration, system, apt, yum, dnf, apk) for selective execution.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: Testing strategy uses a three-layer approach: unit tests (parser, logic), integration tests (real command output), and system tests (privileged operations), with Docker-based multi-OS testing for reproducibility.
Applied to files:
testing/docker/README.md
📚 Learning: 2025-06-24T05:54:21.903Z
Learnt from: CR
PR: bluet/syspkg#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-24T05:54:21.903Z
Learning: For multi-OS and package manager testing, use Docker-based workflows and configuration-driven matrices to ensure comprehensive coverage.
Applied to files:
testing/docker/README.md
🔇 Additional comments (1)
testing/docker/ubuntu.Dockerfile (1)
17-25: Nice TARGETARCH/GO_VERSION install guardThe BuildKit guard plus architecture-aware tarball selection closes the loop for Apple Silicon builders—looks solid.
There was a problem hiding this comment.
Pull Request Overview
This PR adds ARM64 architecture support to Docker-based testing environments to enable testing on both x86_64 and ARM-based systems. The change resolves exec format errors that occurred when running Docker tests on ARM machines like Apple Silicon Macs.
- Updated all Dockerfiles to auto-detect architecture using Docker's TARGETARCH argument
- Modified test fixtures to use vim-enhanced package for YUM-based distributions
- Updated documentation to reflect ARM64 support
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testing/docker/ubuntu.Dockerfile | Added architecture detection and dynamic Go binary download for ARM64/AMD64 |
| testing/docker/rockylinux.Dockerfile | Added architecture detection and dynamic Go binary download for ARM64/AMD64 |
| testing/docker/alpine.Dockerfile | Added architecture detection and dynamic Go binary download for ARM64/AMD64 |
| testing/docker/almalinux.Dockerfile | Added architecture detection and dynamic Go binary download for ARM64/AMD64 |
| testing/docker/docker-compose.test.yml | Updated YUM test fixtures to use vim-enhanced package |
| testing/docker/README.md | Added architecture support documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
thanks for the PR @brunodam ❤️ |
Description
The current Docker-based testing setup is designed for x86_64/amd64 environments. On ARM-based systems (e.g., Apple Silicon Macs, Raspberry Pi, Graviton), the
make test-docker-alltarget cannot run properly because the base images and binaries are not built for ARM.Example
On an ARM machine:
fails with errors such as:
exec format errorExpected
make test-docker-ubuntushould run successfully on both amd64 and arm64 hosts.Contributors on Intel or ARM servers should be able to build and run tests without extra steps.
Actual
make test-docker-ubuntufails on ARM hosts due to architecture mismatch.Notes
Important
Add ARM64 support to Docker test environments by auto-detecting architecture in Dockerfiles and updating documentation.
almalinux.Dockerfile,alpine.Dockerfile,rockylinux.Dockerfile, andubuntu.Dockerfile.README.mdto include ARM64 support for Ubuntu, Alpine, and Fedora.This description was created by
for bdbcc36. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Tests