Use the existing docker-buildx Makefile target for cloudbuilds too instead of push-images python script#225
Use the existing docker-buildx Makefile target for cloudbuilds too instead of push-images python script#225creydr wants to merge 2 commits into
Conversation
✅ Deploy Preview for mcp-lifecycle-operator ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: creydr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
needs-ok-to-test
|
Hi @creydr. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
…er-buildx target The cloudbuild.yaml previously called a Python script (dev/tools/push-images) to build and push multi-arch Docker images. The Makefile already had a docker-buildx target doing essentially the same thing, so this consolidates onto the Makefile target and removes the redundant Python code. Changes: - Add IMAGE_TAG_BASE, IMAGE_TAG, and GIT_IMAGE_TAG variables to the Makefile so IMG is derived from them and CI builds can reference GIT_IMAGE_TAG for date-and-commit based tags. - Extend docker-buildx to support EXTRA_TAGS for applying additional tags in a single build. - Simplify docker-buildx by removing the Dockerfile.cross sed workaround (the Dockerfile already handles TARGETOS/TARGETARCH natively). - Update cloudbuild.yaml to call make docker-buildx directly. - Delete dev/tools/push-images and dev/tools/shared/.
creydr
force-pushed
the
simplify-cloudbuild
branch
from
7f590b3 to
6201af5
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates Docker image building from a Python CLI script to a Makefile-based docker-buildx approach. The Makefile now parameterizes image naming with configurable base name and tag variables, the docker-buildx target applies those variables plus any extra tags, and Cloud Build is updated to invoke the new Make command instead of the Python tool. ChangesImage Build Toolchain Migration
🎯 2 (Simple) | ⏱️ ~10 minutes sequenceDiagram
participant CloudBuild
participant Makefile as Makefile/docker-buildx
participant Buildx as Docker Buildx
participant Registry as Image Registry
CloudBuild->>Makefile: invoke `make docker-buildx` with IMAGE_TAG_BASE, IMAGE_TAG=$(GIT_IMAGE_TAG), EXTRA_TAGS
Makefile->>Buildx: run `buildx build --push` tagging ${IMG} and $(IMAGE_TAG_BASE):$(tag)
Buildx->>Registry: push image(s)
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
11-11: ⚡ Quick winFreeze
GIT_IMAGE_TAGwith:=to avoid re-running$(shell ...)Line 11 uses recursive expansion (
=), so$(shell date ...)/$(shell git describe ...)are re-executed each timeGIT_IMAGE_TAGis referenced. A simply-expanded assignment (:=) evaluates once and keeps the tag stable for the wholemakerun.♻️ Proposed change
-GIT_IMAGE_TAG = v$(shell date +%Y%m%d)-$(shell git describe --always --dirty) +GIT_IMAGE_TAG := v$(shell date +%Y%m%d)-$(shell git describe --always --dirty)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 11, The Makefile currently defines GIT_IMAGE_TAG with recursive expansion using "GIT_IMAGE_TAG = ...", causing the shell commands date and git describe to run every time the variable is referenced; change the assignment to a simply-expanded form "GIT_IMAGE_TAG :=" so the shell substitutions (date and git describe) are evaluated once at assignment and the image tag remains stable for the entire make invocation, updating the variable definition for GIT_IMAGE_TAG accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 196: The docker-buildx invocation is prefixed with a dash so Make ignores
non-zero exits; edit the Makefile and remove the leading '-' before the
$(CONTAINER_TOOL) buildx build --push --platform=$(PLATFORMS) --tag ${IMG}
$(foreach tag,$(EXTRA_TAGS),-t $(IMAGE_TAG_BASE):$(tag)) . command (the
docker-buildx/buildx build --push invocation) so that a failed buildx push
returns a non-zero exit and fails the target/CI.
---
Nitpick comments:
In `@Makefile`:
- Line 11: The Makefile currently defines GIT_IMAGE_TAG with recursive expansion
using "GIT_IMAGE_TAG = ...", causing the shell commands date and git describe to
run every time the variable is referenced; change the assignment to a
simply-expanded form "GIT_IMAGE_TAG :=" so the shell substitutions (date and git
describe) are evaluated once at assignment and the image tag remains stable for
the entire make invocation, updating the variable definition for GIT_IMAGE_TAG
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 017709b4-2acd-4ff8-b0a4-1378a2eeba2d
📒 Files selected for processing (5)
Makefilecloudbuild.yamldev/tools/push-imagesdev/tools/shared/__init__.pydev/tools/shared/utils.py
💤 Files with no reviewable changes (3)
- dev/tools/shared/init.py
- dev/tools/shared/utils.py
- dev/tools/push-images
Remove the leading dash from the buildx build command so a failed build or push propagates as a non-zero exit code instead of being silently ignored.
| .PHONY: docker-buildx | ||
| docker-buildx: ## Build and push docker image for the manager for cross-platform support | ||
| # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile | ||
| sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross |
There was a problem hiding this comment.
@creydr maybe we want to add this into the Dockerfile itself?
My understanding here is this should improve our build times since go will natively cross compile?
3 participants
Summary
The
cloudbuild.yamluses a Python script (dev/tools/push-images) to build and push multi-arch Docker images, duplicating logic already present in the Makefile'sdocker-buildxtarget.This PR addresses it by reusing the existing Makefile target in the cloudbuild too.
Summary by CodeRabbit