-
Notifications
You must be signed in to change notification settings - Fork 562
Add multi-architecture support (AMD64 and ARM64) #177
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: master
Are you sure you want to change the base?
Conversation
enjoy2000
commented
Oct 10, 2025
- Updated tasks.py
- Modified build and push functions to support multi-platform builds using Docker Buildx
- Added a new buildx task that combines building and pushing for efficiency
- Default platforms are linux/amd64,linux/arm64 but can be customized
- Created GitHub Actions workflows
- Updated existing dockerimage.yml to build multi-arch images on CI
- Created new release.yml workflow for building and pushing multi-arch images to Docker Hub
- Verified Dockerfile compatibility
- The Dockerfile is fully compatible with ARM64 as it compiles Redis from source
- Updated README documentation
- Added comprehensive multi-architecture support section
- Included examples for using pre-built images and building locally
- Documented both invoke tasks and direct docker buildx usage
|
Hi @Grokzen , sorry for a lot of vibe code xD. I tested the pipeline here: https://github.com/enjoy2000/docker-redis-cluster/actions/runs/18396534629/job/52417020234 |
|
@enjoy2000 It looks plausible that it could work. The initial thoughts that you MUST remove from it is the test scripts and test parts. Invoke scripts is not something that you generally slap unittests on. The bash script is almost not needed as long as there is github actions to provide integration tests by running buildx. But the python unit ones needs to go. And i don't mind the vibe parts, but I will have to review the new arguments into invoke much more in depht to see if they fit with my general style as i do not really want additional arguments always into the invoke calls if not needed. Possibly some other mechanism where i can define what is supported elsewhere for platforms as they also need support in how they are built and configured on my system to allow for building other platforms, then maybe use the same left to right logic to select platforms by specifying like only arm64 for it. The other main thing that will block this MR is that all commits is done by Claude as a user and not by you as a user. If i can't track and show who acctually did them, that will be a no-go for me and i might just take this and base my own version, but i would rather want your information in the commits instead. |
|
Also note that you should at least be in-line with my other MR that will be merged this weekend probably https://github.com/Grokzen/docker-redis-cluster/pull/176/files that contains lots of changes. Probably should rebase this branch or remake it on-top of my other changes as you will at some point have to align up as this MR will not be merged before that one. |
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.
These things need to be addressed. It is in the right direction but you missed and modified a fair bit of core ideas that I put into the code earlier that needs to remain.
.github/workflows/release.yml
Outdated
| @@ -0,0 +1,68 @@ | |||
| name: Release Multi-Arch Images | |||
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.
A release workflow also gave to be ejected. That is something that I will build myself if I want to in the future. Rn i only allow uploads for releases outside CI.
.github/workflows/dockerimage.yml
Outdated
| - name: Install Python dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install invoke requests |
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.
At least in my other MR I bundle dependencies in a requirements file. If you renade ontop of the MRnthis should be changed to req file.
run_tests.sh
Outdated
| echo "Running tests for tasks.py..." | ||
|
|
||
| # Install test dependencies if needed | ||
| pip install -q invoke requests |
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.
Same here for requirements
tasks.py
Outdated
| @task | ||
| def push(c, version, cpu=None): | ||
| print(f" -- Docker push version to docker-hub : {version}") | ||
| def push(c, version, cpu=None, platforms="linux/amd64,linux/arm64"): |
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.
From a argument pov that I would have to provide multiple arguments with, to build multiple is bad and it breaks the other method and flow with left to right support. What I choose must be simpler like arm64 only and then you can find/expand it to what buildx needs inside the code with a dict/mapping table.
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.
updated this
|
Thanks @Grokzen, for detailed feedback. I will update them and wait for your other MR to get merged. And I will rebase on that later. |