-
Notifications
You must be signed in to change notification settings - Fork 119
[dx] Remove BUILDER and PLATFORM autodetection logic #1391
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
Conversation
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
WalkthroughMakefile targets across apps, core, extra, and system replace explicit docker buildx flags with a single $(BUILDX_ARGS). scripts/common-envs.mk introduces TAG/BUILDER/PLATFORM/BUILDX_EXTRA_ARGS and composes BUILDX_ARGS, removing previous builder/platform auto-detection. Existing tagging, caching, metadata, and post-build digest handling remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev/CI
participant Make as Makefile Target
participant Env as scripts/common-envs.mk
participant Buildx as docker buildx
participant Reg as Registry
Dev/CI->>Make: make image-*
Make->>Env: include and resolve TAG/BUILDER/PLATFORM/BUILDX_ARGS
Env-->>Make: export BUILDX_ARGS (provenance/push/load/label + optional builder/platform + extra)
Make->>Buildx: buildx build $(BUILDX_ARGS) --tag ... --cache-* --metadata-file ...
alt push enabled
Buildx->>Reg: push image
else load enabled
Buildx-->>Make: load image locally
end
Buildx-->>Make: metadata (digest)
Make->>Make: parse digest, write tag files / update values.yaml
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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 of Changes
Hello @kvaps, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the image building process by centralizing Docker Buildx arguments into a single variable and removing the automatic detection of builder and platform settings. The change aims to simplify and standardize build commands across various components, making the build system more maintainable and predictable.
Highlights
- Buildx Argument Consolidation: The pull request introduces a new
BUILDX_ARGS
variable inscripts/common-envs.mk
to centralize and manage commondocker buildx build
arguments. This simplifies individual Makefile targets by replacing multiple explicit flags with a single variable. - Removal of Autodetection Logic: The previous autodetection logic for
BUILDER
andPLATFORM
variables, which relied ondocker buildx inspect
andjq
, has been removed fromscripts/common-envs.mk
. These variables are now optionally set, and if not provided,buildx
will use its default behavior. - Streamlined Makefile Targets: Across numerous Makefile targets responsible for building container images, redundant
docker buildx build
flags such as--provenance
,--builder
,--platform
,--push
,--label
, and--load
have been removed and replaced with the new$(BUILDX_ARGS)
variable. This significantly cleans up and standardizes the image build commands.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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
This pull request refactors the Docker image build process by removing the automatic detection of BUILDER
and PLATFORM
from the Makefiles. Instead, it introduces a centralized BUILDX_ARGS
variable in scripts/common-envs.mk
to hold common docker buildx
arguments. This change simplifies the individual Makefiles, reduces code duplication, and makes the build process more explicit and maintainable. The refactoring is applied consistently across all relevant packages. As a positive side effect, this change also fixes latent bugs in a few Makefiles where build arguments were incorrectly specified.
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/dashboard/Makefile (1)
21-27
: BUG: ‘tag’ variable assignment is commented out, subsequent uses are empty
Lines 22 and 26 use $${tag} but Line 21 is commented, so tag is unset → wget/grep/sed break.Apply:
- #tag=$$(git ls-remote --tags --sort="v:refname" https://github.com/vmware-tanzu/kubeapps | awk -F'[/^]' 'END{print $$3}') && \ + tag=$$(git ls-remote --tags --sort="v:refname" https://github.com/vmware-tanzu/kubeapps | awk -F'[/^]' 'END{print $$3}') && \Optional hardening:
- wget https://github.com/vmware-tanzu/kubeapps/raw/$${tag}/cmd/kubeapps-apis/Dockerfile -O images/kubeapps-apis/Dockerfile && \ + test -n "$$tag" || { echo "No kubeapps tag found"; exit 1; } ; \ + wget https://github.com/vmware-tanzu/kubeapps/raw/$${tag}/cmd/kubeapps-apis/Dockerfile -O images/kubeapps-apis/Dockerfile && \
🧹 Nitpick comments (8)
scripts/common-envs.mk (2)
5-7
: Expose new knobs clearly (docs/help)
With autodetection removed, users must set BUILDER/PLATFORM explicitly. Please add a short README blurb or a make “help” target showing examples:
- BUILDER=mybuilder
- PLATFORM=linux/amd64,linux/arm64
- BUILDX_EXTRA_ARGS=--progress=plain
6-6
: Strip trailing whitespace
Minor style nit; “PLATFORM ?=” currently has a trailing space which is easy to propagate in diffs.-PLATFORM ?= +PLATFORM ?=packages/apps/mysql/Makefile (1)
15-15
: Consistent adoption of $(BUILDX_ARGS) — check digest JSON compatibilityLooks good. Ensure the metadata JSON still contains "containerimage.digest" under your current buildx/provenance settings; otherwise yq extraction will break.
If you foresee per-target extras (e.g., build-arg overrides), confirm BUILDX_ARGS appends BUILDX_EXTRA_ARGS so targets can opt-in without touching Makefiles.
packages/core/testing/Makefile (1)
23-23
: e2e-sandbox developer UX — default to --load for local runsFor this dev-heavy target, ensure BUILDX_ARGS chooses --load by default when REGISTRY or PUSH isn’t set, so docker run later can pull from the local daemon without requiring a registry push.
Document in scripts/common-envs.mk how to flip between load/push (e.g., PUSH=1 make image-e2e-sandbox).
packages/extra/monitoring/Makefile (1)
17-17
: Centralized flags: parity check and minor Make hygieneChange looks fine; same parity checks as other targets apply.
Consider marking phony targets to avoid timestamp pitfalls:
- .PHONY: image generate
Optional, low impact.
packages/system/kamaji/Makefile (1)
20-20
: Add a fail-fast guard for missing containerimage.digest
Ensureimages/kamaji.json
contains.containerimage.digest
after the build to avoid writing an invalid tag. For example, right before updatingvalues.yaml
add:DIGEST="$(yq -r '."containerimage.digest" // empty' images/kamaji.json)" [ -n "$DIGEST" ] || { echo "Missing containerimage.digest (did you --push?)"; exit 1; }packages/apps/http-cache/Makefile (1)
14-14
: Same here: keep command robust if BUILDX_ARGS is empty and validate digest availability
- Prevent a stray backslash if BUILDX_ARGS is empty by moving it before the last flag.
- --metadata-file images/nginx-cache.json \ - $(BUILDX_ARGS) + $(BUILDX_ARGS) \ + --metadata-file images/nginx-cache.json
- The subsequent echo relies on containerimage.digest. Verify BUILDX_ARGS ensures a push/registry export; otherwise images/nginx-cache.json may lack the digest.
packages/system/cozystack-api/Makefile (1)
15-15
: Consolidation looks good; add two sanity checks
- Move BUILDX_ARGS before the final flag to avoid a trailing backslash hazard if it’s empty.
- --metadata-file images/cozystack-api.json \ - $(BUILDX_ARGS) + $(BUILDX_ARGS) \ + --metadata-file images/cozystack-api.json
- Ensure scripts/common-envs.mk composes BUILDX_ARGS with:
- a source label (org.opencontainers.image.source) and desired provenance policy,
- mutually compatible options (e.g., disallow --load with multi-arch --platform lists),
- a push/registry export so images/cozystack-api.json has containerimage.digest used in values.yaml.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
packages/apps/clickhouse/Makefile
(1 hunks)packages/apps/http-cache/Makefile
(1 hunks)packages/apps/kubernetes/Makefile
(2 hunks)packages/apps/mysql/Makefile
(1 hunks)packages/core/installer/Makefile
(2 hunks)packages/core/testing/Makefile
(1 hunks)packages/extra/monitoring/Makefile
(1 hunks)packages/system/bucket/Makefile
(1 hunks)packages/system/cilium/Makefile
(1 hunks)packages/system/cozystack-api/Makefile
(1 hunks)packages/system/cozystack-controller/Makefile
(1 hunks)packages/system/dashboard/Makefile
(2 hunks)packages/system/kamaji/Makefile
(1 hunks)packages/system/kubeovn-webhook/Makefile
(1 hunks)packages/system/kubeovn/Makefile
(1 hunks)packages/system/metallb/Makefile
(1 hunks)packages/system/objectstorage-controller/Makefile
(1 hunks)scripts/common-envs.mk
(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/system/dashboard/images/kubeapps-apis/Dockerfile:15-17
Timestamp: 2025-05-14T16:58:01.478Z
Learning: When working with Dockerfiles for cross-architecture builds in the kubeapps-apis component, the `TARGETOS` and `TARGETARCH` build arguments should be declared without default values so that Docker's buildx system can automatically set these variables based on the specified target platform.
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/core/testing/images/e2e-sandbox/Dockerfile:7-8
Timestamp: 2025-05-14T16:57:29.651Z
Learning: In Dockerfiles supporting cross-architecture builds, `TARGETOS` and `TARGETARCH` are special build arguments that should be declared without default values. Docker's buildx system automatically sets these variables based on the specified target platform during cross-platform builds.
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/apps/kubernetes/images/cluster-autoscaler/Dockerfile:3-4
Timestamp: 2025-05-14T16:54:50.368Z
Learning: In Dockerfiles for the cozystack project, ARG ordering where a variable like ${TARGETARCH} is referenced before being declared can still work correctly in practice because Docker's BuildKit may evaluate the variable only when it's actually used in a FROM statement.
📚 Learning: 2025-05-14T16:58:01.478Z
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/system/dashboard/images/kubeapps-apis/Dockerfile:15-17
Timestamp: 2025-05-14T16:58:01.478Z
Learning: When working with Dockerfiles for cross-architecture builds in the kubeapps-apis component, the `TARGETOS` and `TARGETARCH` build arguments should be declared without default values so that Docker's buildx system can automatically set these variables based on the specified target platform.
Applied to files:
packages/apps/http-cache/Makefile
packages/system/cilium/Makefile
packages/system/kubeovn-webhook/Makefile
packages/extra/monitoring/Makefile
packages/system/objectstorage-controller/Makefile
packages/apps/mysql/Makefile
packages/system/cozystack-api/Makefile
packages/apps/clickhouse/Makefile
packages/core/installer/Makefile
packages/system/dashboard/Makefile
packages/system/kamaji/Makefile
packages/system/cozystack-controller/Makefile
scripts/common-envs.mk
packages/system/kubeovn/Makefile
packages/system/metallb/Makefile
packages/apps/kubernetes/Makefile
📚 Learning: 2025-05-14T16:57:29.651Z
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/core/testing/images/e2e-sandbox/Dockerfile:7-8
Timestamp: 2025-05-14T16:57:29.651Z
Learning: In Dockerfiles supporting cross-architecture builds, `TARGETOS` and `TARGETARCH` are special build arguments that should be declared without default values. Docker's buildx system automatically sets these variables based on the specified target platform during cross-platform builds.
Applied to files:
packages/system/cilium/Makefile
packages/extra/monitoring/Makefile
packages/core/installer/Makefile
packages/system/dashboard/Makefile
scripts/common-envs.mk
packages/system/metallb/Makefile
📚 Learning: 2025-05-14T16:54:50.368Z
Learnt from: nbykov0
PR: cozystack/cozystack#932
File: packages/apps/kubernetes/images/cluster-autoscaler/Dockerfile:3-4
Timestamp: 2025-05-14T16:54:50.368Z
Learning: In Dockerfiles for the cozystack project, ARG ordering where a variable like ${TARGETARCH} is referenced before being declared can still work correctly in practice because Docker's BuildKit may evaluate the variable only when it's actually used in a FROM statement.
Applied to files:
packages/system/cozystack-api/Makefile
packages/system/cozystack-controller/Makefile
scripts/common-envs.mk
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (14)
scripts/common-envs.mk (1)
2-2
: LGTM: TAG computation
Exact-match tag or “latest” fallback is sane and consistent with existing targets.packages/system/dashboard/Makefile (2)
35-35
: LGTM: Centralized Buildx flags
Switching to $(BUILDX_ARGS) keeps the command concise and consistent.
52-52
: LGTM: Centralized Buildx flags
Consistent with the repo-wide pattern.packages/system/cozystack-controller/Makefile (1)
15-15
: LGTM: $(BUILDX_ARGS) adoption
Keeps flags centralized; digest wiring remains intact.packages/system/objectstorage-controller/Makefile (1)
25-25
: LGTM: $(BUILDX_ARGS) adoption for multi-target build
Consistent with the refactor; TARGET/stage selection unaffected.packages/system/kubeovn/Makefile (1)
27-27
: LGTM: $(BUILDX_ARGS) adoption
Centralizes build flags; existing dual tagging and digest update remain unchanged.packages/apps/kubernetes/Makefile (1)
21-21
: Centralized Buildx flags verified
All build targets now use$(BUILDX_ARGS)
exclusively;scripts/common-envs.mk
defines--provenance=false
,--push/--load
(defaults to exactly one) and optional--platform
/--builder
, with no residual flags in individual Makefiles.packages/system/cilium/Makefile (1)
25-25
: No changes needed: BUILDX_ARGS doesn’t include or override--metadata-file
, and the explicit--metadata-file images/cilium.json
remains in the Makefile.packages/apps/clickhouse/Makefile (1)
15-15
: Verify default values for PUSH/LOAD
BUILDX_ARGS uses--push=$(PUSH)
and--load=$(LOAD)
as defined inscripts/common-envs.mk
. Ensure thatPUSH
andLOAD
default to the same values as before (e.g., CI wasn’t implicitly settingPUSH=true
orLOAD=false
). If previous CI runs relied on unconditionally pushing, explicitly setPUSH=1
(ortrue
) in the Make invocation.packages/system/metallb/Makefile (1)
24-24
: Ensure CI explicitly passes PLATFORM or BUILDX_EXTRA_ARGS for multi-arch builds
The default PLATFORM is unset (see common-envs.mk), so$(BUILDX_ARGS)
won’t include--platform=…
unless CI exports PLATFORM or adds it via BUILDX_EXTRA_ARGS. Verify your pipeline invokes make with PLATFORM set (or injects it through BUILDX_EXTRA_ARGS) to restore multi-arch outputs.packages/system/bucket/Makefile (1)
19-19
: Add PUSH/LOAD environment variables to CI workflow
I don’t see PUSH or LOAD defined in any.github/workflows/*.yml
; add them underenv:
in your build job soBUILDX_ARGS := --push=$(PUSH) --load=$(LOAD)…
receives the correct modes.packages/system/kubeovn-webhook/Makefile (1)
13-13
: BUILDX_ARGS parity verified — no changes needed. Definition in scripts/common-envs.mk still includes--provenance=false
, theorg.opencontainers.image.source
label, respectsPUSH
/LOAD
defaults, and conditionally adds--builder
/--platform
.packages/core/installer/Makefile (2)
31-31
: image-cozystack: Confirm push/load behavior under $(BUILDX_ARGS)This target feeds values.yaml used by apply flows; a silent switch from --load to --push (or vice versa) would be disruptive. Verify BUILDX_ARGS defaults match prior behavior used by contributors running this target locally.
Reuse the script from the kubeovn-webhook comment to print PUSH/LOAD defaults and BUILDX_ARGS.
49-49
: image-matchbox: Multi-tag build — ensure digest mapping is stableWith two --tag flags, the metadata-file should still report a single image digest. Validate that:
- images/matchbox.json includes containerimage.digest,
- both tags reference the same digest (buildx manifest logic unchanged).
After a dry run in CI or locally, cat images/matchbox.json and confirm the yq path resolves and matches the pushed tag(s).
BUILDX_ARGS := --provenance=false --push=$(PUSH) --load=$(LOAD) \ | ||
--label org.opencontainers.image.source=https://github.com/cozystack/cozystack \ | ||
$(if $(strip $(BUILDER)),--builder=$(BUILDER)) \ | ||
$(if $(strip $(PLATFORM)),--platform=$(PLATFORM)) \ | ||
$(BUILDX_EXTRA_ARGS) |
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.
🛠️ Refactor suggestion
Make --push/--load mutually exclusive and avoid boolean flags with values
Buildx treats these as presence flags; passing both or using “=1/0” can misbehave. Gate each flag and error if both are enabled.
Apply within this hunk:
-BUILDX_ARGS := --provenance=false --push=$(PUSH) --load=$(LOAD) \
+BUILDX_ARGS := --provenance=false \
+ $(if $(filter 1 true yes,$(PUSH)),--push) \
+ $(if $(filter 1 true yes,$(LOAD)),--load) \
--label org.opencontainers.image.source=https://github.com/cozystack/cozystack \
$(if $(strip $(BUILDER)),--builder=$(BUILDER)) \
$(if $(strip $(PLATFORM)),--platform=$(PLATFORM)) \
$(BUILDX_EXTRA_ARGS)
Add guard (outside this hunk, e.g., after Line 7):
# Ensure only one of PUSH/LOAD is enabled
ifneq ($(and $(filter 1 true yes,$(PUSH)),$(filter 1 true yes,$(LOAD))),)
$(error Both PUSH and LOAD are enabled; enable only one of PUSH or LOAD)
endif
🤖 Prompt for AI Agents
In scripts/common-envs.mk around lines 10 to 14, the Makefile currently passes
--push=$(PUSH) and --load=$(LOAD) which treats these as boolean-with-value flags
and can result in both being passed; add a guard after line 7 that errors when
both PUSH and LOAD are truthy (1/true/yes) to make them mutually exclusive, and
change the BUILDX_ARGS hunk to emit presence-only flags (e.g., use conditionals
to include --push when PUSH is set and --load when LOAD is set) instead of using
=$(PUSH)/=$(LOAD) so buildx receives the flags only when enabled.
💡 Verification agent
🧩 Analysis chain
Repo-wide adoption sanity check
Run this to confirm no stragglers keep passing buildx flags directly and all builds include $(BUILDX_ARGS).
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find buildx calls missing BUILDX_ARGS (might show context next line):"
rg -nP --type=make -C2 'docker\s+buildx\s+build(?!.*\$\((?:BUILDX_ARGS)\))'
echo -e "\n2) Find direct flag usage outside scripts/common-envs.mk:"
rg -nP --type=make -g '!scripts/common-envs.mk' -- '(--platform|--builder|--provenance|--push(=| )|--load(=| ))'
Length of output: 9489
Include $(BUILDX_ARGS)
in every docker buildx build
invocation
Numerous Makefiles (e.g. packages/system/cozystack-api/Makefile:10
, packages/system/objectstorage-controller/Makefile:19
, packages/system/metallb/Makefile:17
, and many more under packages/…
) still call docker buildx build
with hard-coded flags instead of using the shared $(BUILDX_ARGS)
; update them to docker buildx build $(BUILDX_ARGS) …
.
🤖 Prompt for AI Agents
In scripts/common-envs.mk around lines 10 to 14, the shared BUILDX_ARGS variable
is defined but many Makefiles still call docker buildx build with hard-coded
flags; update every docker buildx build invocation across the repo (e.g.,
packages/system/cozystack-api/Makefile:10,
packages/system/objectstorage-controller/Makefile:19,
packages/system/metallb/Makefile:17 and other packages/* Makefiles) to use
docker buildx build $(BUILDX_ARGS) … instead of repeating flags, ensuring any
additional per-target flags are appended after $(BUILDX_ARGS).
Successfully created backport PR for |
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
It does not work well anyway.
Release note
Summary by CodeRabbit