-
Notifications
You must be signed in to change notification settings - Fork 0
support ARM64 with official Ruby + jemalloc #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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:
- Security vulnerability in Dockerfile: The wildcard pattern in the jemalloc symbolic link could cause unpredictable behavior across architectures
- 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
- 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.
|
[ch97578] |
|
/q review |
There was a problem hiding this 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
-
Dockerfile: The jemalloc symbolic link creation could fail silently, causing runtime crashes when
LD_PRELOADattempts to load a missing library. -
sync-to-ecr.sh: The script assumes both base and
-testingtags 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.
|
/q review |
There was a problem hiding this 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:
- Dockerfile: Unsafe script download without integrity checks
- 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.
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
linux/amd64和linux/arm64sync-to-ecr.sh (新增)
README.md
Release Flow
Related