Skip to content

Conversation

@deltaguita
Copy link

@deltaguita deltaguita commented Dec 31, 2025

Summary

支援 ARM64 架構,讓 iCook、tv、market、newsroom 或其他 rails 專案可以在 Graviton instances 上運行。

Changes

Dockerfile

  • 改用官方 ruby:2.7.8-slim 取代 fullstaq-ruby(fullstaq 不支援 ARM64)
  • 手動安裝 libjemalloc2 + LD_PRELOAD 達到相同效果

release.yml

  • 加入 QEMU + buildx 支援 multi-arch build
  • 同時 build linux/amd64linux/arm64
  • 更新 actions 版本

sync-to-ecr.sh (新增)

  • 同步 DockerHub image 到 ECR Public
  • 支援 amd64 和 arm64 (目前 production 還需要 amd64)

README.md

  • 新增 ARM64 支援說明
  • 新增發版流程

Release Flow

# 1. Merge PR
# 2. Create tag
git tag 0.32.0
git push origin 0.32.0

# 3. Wait for CI, then sync to ECR
./sync-to-ecr.sh 0.32.0

Related

Copy link

@amazon-q-developer amazon-q-developer bot left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR successfully adds ARM64 support to the Docker base image, which is a valuable enhancement for running on Graviton instances. However, there are several critical issues that need to be addressed before merging:

Critical Issues Found:

  1. Security vulnerability in Dockerfile: The wildcard pattern in the jemalloc symbolic link could cause unpredictable behavior across architectures
  2. Logic error in sync script: The ARM64 tagging approach breaks Docker's multi-architecture support by creating separate tags instead of proper multi-arch manifests
  3. Missing error handling: The sync script lacks proper error handling for Docker operations

Performance Improvements:

  • GitHub Actions workflow should include build caching to improve multi-architecture build times
  • Documentation should use generic version placeholders instead of hardcoded version numbers

Overall Assessment:

The core changes (switching from fullstaq-ruby to official Ruby + jemalloc, adding multi-arch CI support) are well-implemented. The main concerns are around the deployment script architecture handling and some security/reliability improvements needed in the Dockerfile.

Please address the critical issues, particularly the jemalloc symbolic link and the multi-architecture manifest handling in the sync script, before merging.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

@deltaguita deltaguita changed the title feat: support ARM64 with official Ruby + jemalloc support ARM64 with official Ruby + jemalloc Dec 31, 2025
@deltaguita
Copy link
Author

[ch97578]

@deltaguita
Copy link
Author

/q review

Copy link

@amazon-q-developer amazon-q-developer bot left a comment

Choose a reason for hiding this comment

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

Summary

This PR successfully adds ARM64 support by migrating from fullstaq-ruby to the official Ruby image with manual jemalloc installation. The changes are well-structured and the multi-architecture build setup is appropriate.

Critical Issues Found

  1. Dockerfile: The jemalloc symbolic link creation could fail silently, causing runtime crashes when LD_PRELOAD attempts to load a missing library.

  2. sync-to-ecr.sh: The script assumes both base and -testing tags exist without verification, which will cause sync failures if only the base version is available.

Recommendations

  • Add explicit error handling for jemalloc library detection in the Dockerfile
  • Include build caching in the GitHub Actions workflow to improve multi-arch build performance
  • Add tag existence verification in the ECR sync script
  • Update documentation to include script permission setup

The ARM64 support implementation is solid, but these reliability improvements are essential for production deployment.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

@deltaguita
Copy link
Author

/q review

Copy link

@amazon-q-developer amazon-q-developer bot left a comment

Choose a reason for hiding this comment

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

Code Review Summary

This PR successfully adds ARM64 support to the Docker base image by switching from fullstaq-ruby to the official Ruby image and implementing jemalloc via LD_PRELOAD. The multi-architecture build setup and ECR sync script are well-implemented.

Critical Security Issues Found

I've identified 3 security vulnerabilities that must be addressed before merging:

  1. Dockerfile: Unsafe script download without integrity checks
  2. sync-to-ecr.sh: Unquoted shell variables creating command injection risks (2 instances)

All issues have specific code suggestions provided for easy fixes. Please apply the suggested changes to resolve these security concerns.

Overall Assessment

The architectural changes are sound and the ARM64 support implementation is appropriate. Once the security issues are resolved, this PR will be ready for merge.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants