build: support patio arm64 image#258
Conversation
Signed-off-by: TzZtzt <[email protected]>
Signed-off-by: TzZtzt <[email protected]>
Signed-off-by: TzZtzt <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the Docker build process in the Makefile by removing the dynamic generation of cross-build Dockerfiles and introduces a new target for the patio image. However, the use of --platform=$BUILDPLATFORM in the patio Dockerfile is incorrect for single-stage builds and will lead to runtime errors on non-host architectures. Feedback also includes improving the Makefile documentation by adding help descriptions for the new and updated targets.
| - $(CONTAINER_TOOL) buildx rm rbgs-crd-builder | ||
|
|
||
| .PHONY: docker-buildx-push-patio | ||
| docker-buildx-push-patio: |
There was a problem hiding this comment.
The new target docker-buildx-push-patio is missing a help description. Please add a ## comment so it appears when running make help.
docker-buildx-push-patio: ## Build and push Patio image for cross-platform support
|
|
||
| .PHONY: docker-buildx-push | ||
| docker-buildx-push: docker-buildx docker-buildx-push-crd-upgrader ## Build and push multi-arch controller and CRD upgrader images | ||
| docker-buildx-push: docker-buildx docker-buildx-push-crd-upgrader docker-buildx-push-patio ## Build and push multi-arch controller and CRD upgrader images |
There was a problem hiding this comment.
The help description for docker-buildx-push should be updated to reflect that it now also builds and pushes the patio image.
docker-buildx-push: docker-buildx docker-buildx-push-crd-upgrader docker-buildx-push-patio ## Build and push multi-arch controller, CRD upgrader, and patio images
Signed-off-by: TzZtzt <[email protected]>
Signed-off-by: TzZtzt <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR updates the container build pipeline to publish the Patio runtime image as a multi-arch (amd64/arm64) image, aligning it with the existing multi-arch controller/CRD-upgrader publishing flow.
Changes:
- Buildx flow: stop generating
Dockerfile.crossand build the controller directly fromDockerfile. - Add a new
make docker-buildx-push-patiotarget and include it inmake docker-buildx-pushto publish Patio as multi-arch. - GitHub Actions: remove the separate Patio build/push step and reflect Patio as multi-arch in the summary.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
python/patio/Dockerfile |
Minor formatting change; no functional build logic changes. |
Makefile |
Adds Patio buildx push target; simplifies controller buildx by using Dockerfile directly. |
.github/workflows/docker-build-push.yml |
Switches Patio publishing to the unified docker-buildx-push path and updates summary text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "- \`${{ secrets.DOCKERHUB_USERNAME }}/rbgs-controller:${{ steps.version.outputs.tag }}\` (multi-arch)" >> $GITHUB_STEP_SUMMARY | ||
| echo "- \`${{ secrets.DOCKERHUB_USERNAME }}/rbgs-upgrade-crd:${{ steps.version.outputs.tag }}\` (multi-arch)" >> $GITHUB_STEP_SUMMARY | ||
| echo "- \`${{ secrets.DOCKERHUB_USERNAME }}/rbgs-patio-runtime:${{ steps.version.outputs.tag }}\`" >> $GITHUB_STEP_SUMMARY | ||
| echo "- \`${{ secrets.DOCKERHUB_USERNAME }}/rbgs-patio-runtime:${{ steps.version.outputs.tag }}\` (multi-arch)" >> $GITHUB_STEP_SUMMARY | ||
| echo "- \`${{ secrets.DOCKERHUB_USERNAME }}/rbgs-benchmark-dashboard:${{ steps.version.outputs.tag }}\`" >> $GITHUB_STEP_SUMMARY |
There was a problem hiding this comment.
The workflow now relies on make docker-buildx-push, which (per the updated Makefile) also builds/pushes the Patio image. Consider updating the step name/echo text earlier in the job so the workflow logs reflect that Patio is included, to avoid confusion when troubleshooting build failures.
| docker-buildx-push-patio: ## Build and push patio image for cross-platform support | ||
| - $(CONTAINER_TOOL) buildx create --name rbgs-patio-builder | ||
| $(CONTAINER_TOOL) buildx use rbgs-patio-builder | ||
| $(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${PATIO_IMG}:${TAG} $(DOCKER_BUILD_ARGS) -f ${PATIO_DOCKERFILE} . | ||
| - $(CONTAINER_TOOL) buildx rm rbgs-patio-builder |
There was a problem hiding this comment.
docker buildx rm won’t run if the preceding buildx build fails (because Make stops on error), which can leave the named builder around locally and potentially affect later runs. Consider ensuring cleanup runs even on failure (e.g., via a shell trap or a single recipe line that performs build then cleanup).
Ⅰ. Motivation
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #XXXX
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.