Skip to content

Add multi-architecture support (AMD64 and ARM64) - #177

Open
enjoy2000 wants to merge 1 commit into
Grokzen:masterfrom
enjoy2000:master
Open

Add multi-architecture support (AMD64 and ARM64)#177
enjoy2000 wants to merge 1 commit into
Grokzen:masterfrom
enjoy2000:master

Conversation

@enjoy2000

Copy link
Copy Markdown
  1. 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
  1. 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
  1. Verified Dockerfile compatibility
  • The Dockerfile is fully compatible with ARM64 as it compiles Redis from source
  1. 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

@enjoy2000

Copy link
Copy Markdown
Author

Hi @Grokzen , sorry for a lot of vibe code xD.
Could you help to check what could be included, anything needs to be fixed?
ARM64 support will be super helpful to Mac users like me tho.

I tested the pipeline here: https://github.com/enjoy2000/docker-redis-cluster/actions/runs/18396534629/job/52417020234

@Grokzen

Grokzen commented Oct 10, 2025

Copy link
Copy Markdown
Owner

@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.

@Grokzen

Grokzen commented Oct 10, 2025

Copy link
Copy Markdown
Owner

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.

@Grokzen Grokzen left a comment

Copy link
Copy Markdown
Owner

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.

Comment thread .github/workflows/release.yml Outdated
@@ -0,0 +1,68 @@
name: Release Multi-Arch Images

Copy link
Copy Markdown
Owner

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.

Comment thread .github/workflows/dockerimage.yml Outdated
- name: Install Python dependencies
run: |
python -m pip install --upgrade pip
pip install invoke requests

Copy link
Copy Markdown
Owner

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.

Comment thread run_tests.sh Outdated
echo "Running tests for tasks.py..."

# Install test dependencies if needed
pip install -q invoke requests

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Same here for requirements

Comment thread __pycache__/tasks.cpython-312.pyc Outdated
Comment thread run_tests.sh Outdated
Comment thread tasks.py Outdated
Comment thread 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"):

Copy link
Copy Markdown
Owner

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated this

Comment thread tasks.py Outdated
Comment thread tasks.py Outdated
Comment thread test_tasks.py Outdated
@enjoy2000

Copy link
Copy Markdown
Author

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.

@niemyjski

Copy link
Copy Markdown

Any chance we can get this merged?

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.

3 participants