fix: remove helm linux binaries from source repository#6058
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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 updates the build process by adding a step to download Helm in the Makefile and multiple Dockerfiles, while also updating the .gitignore file to ignore the /bin/ directory. The review feedback suggests optimizing the Dockerfiles to leverage layer caching by copying the Makefile and running make download-helm before copying the rest of the source code, which prevents downloading Helm on every code change.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
To leverage Docker's layer caching and avoid downloading the Helm binaries (~30MB) on every single code change, copy only the Makefile first and run make download-helm before copying the rest of the source code. This significantly speeds up subsequent builds when only Go source files are modified.
COPY Makefile Makefile
RUN make download-helm
COPY . .
ARG FLUID_VERSION
Remove pre-committed helm ELF binaries (helm-linux-amd64, helm-linux-arm64) from the repository to resolve Scorecard Binary-Artifacts alerts (fluid-cloudnative#1461, fluid-cloudnative#1462). Helm binaries are now downloaded at build time via `make download-helm` in each Dockerfile's builder stage. Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cheyang
force-pushed
the
remove_helm_linux_binary
branch
from
c66910e to
0964494
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes reliance on pre-committed Helm Linux binaries by switching the build to download Helm during image builds, and updates ignore/build wiring to prevent reintroducing binary artifacts into the repository.
Changes:
- Adds
make download-helmexecution to each controller Dockerfile builder stage to fetch Helm during image build. - Wires
download-helminto aggregate Makefile docker build targets. - Updates
.gitignoreto ignore the top-level/bin/directory (including Helm/tooling outputs).
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Ensures Helm download runs before aggregate docker build/buildx targets. |
| docker/Dockerfile.dataset | Downloads Helm in builder stage before building dataset controller image. |
| docker/Dockerfile.application | Downloads Helm in builder stage before building application controller image. |
| docker/Dockerfile.alluxioruntime | Downloads Helm in builder stage before building alluxio runtime controller image. |
| docker/Dockerfile.cacheruntime | Downloads Helm in builder stage before building cache runtime controller image. |
| docker/Dockerfile.efcruntime | Downloads Helm in builder stage before building EFC runtime controller image. |
| docker/Dockerfile.jindoruntime | Downloads Helm in builder stage before building Jindo runtime controller image. |
| docker/Dockerfile.juicefsruntime | Downloads Helm in builder stage before building JuiceFS runtime controller image. |
| docker/Dockerfile.thinruntime | Downloads Helm in builder stage before building thin runtime controller image. |
| docker/Dockerfile.vineyardruntime | Downloads Helm in builder stage before building Vineyard runtime controller image. |
| .gitignore | Stops tracking/adding build outputs under /bin/ (including Helm downloads). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
| ARG FLUID_VERSION | ||
| RUN make download-helm |
|
|
||
| .PHONY: docker-build-all | ||
| docker-build-all: pre-setup ${DOCKER_BUILD} | ||
| docker-build-all: pre-setup download-helm ${DOCKER_BUILD} |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6058 +/- ##
=======================================
Coverage 64.90% 64.90%
=======================================
Files 480 480
Lines 33554 33554
=======================================
Hits 21779 21779
Misses 10049 10049
Partials 1726 1726 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
When this Dockerfile is built directly with docker build --build-arg HELM_VERSION=<version> (without first running make docker-build-all to populate bin/helm/<version>/ on the host), the builder stage falls back to the Makefile default HELM_VERSION ?= v3.19.5 because no ARG HELM_VERSION is declared before RUN make download-helm. The runtime stage, however, references ${HELM_VERSION} in COPY --from=builder ... /bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} and would look up a directory the builder never created, breaking the build. Could you add ARG HELM_VERSION to the builder stage before RUN make download-helm so that override flows correctly through both stages? Same fix applies to the other 8 Dockerfile.* files in this PR.
|
Thanks for getting these binaries out of the source tree — the diff is mechanical and safe, and the migration to
None of the above blocks the PR; happy to land this once you've had a chance to weigh in on the in-builder |
| COPY . . | ||
|
|
||
| ARG FLUID_VERSION | ||
| RUN make download-helm |
There was a problem hiding this comment.
When this Dockerfile is built directly with docker build --build-arg HELM_VERSION=<version> (without first running make docker-build-all to populate bin/helm/<version>/ on the host), the builder stage has no ARG HELM_VERSION declared, so make download-helm falls back to the Makefile default HELM_VERSION ?= v3.19.5. The runtime stage then uses ${HELM_VERSION} from the build arg in COPY --from=builder ... /bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} and would point at a directory the builder never created, breaking the build. Could you add ARG HELM_VERSION (and ARG TARGETARCH if intended for the for-loop side) to the builder stage before RUN make download-helm? The same change should be replicated across the other 8 docker/Dockerfile.* files updated in this PR.
…stage Without ARG HELM_VERSION in the builder stage, make download-helm uses the Makefile default while the runtime stage COPY references the build arg value, causing path mismatch when HELM_VERSION is overridden. Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy only the Makefile before running `make download-helm` so that the helm download layer (~30MB) is cached and not invalidated by source code changes. This speeds up subsequent image builds when only Go files are modified. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
cheyang
force-pushed
the
remove_helm_linux_binary
branch
from
a171e3d to
dff2976
Compare
| WORKDIR /go/src/github.com/fluid-cloudnative/fluid | ||
| COPY Makefile Makefile | ||
|
|
||
| ARG HELM_VERSION |
There was a problem hiding this comment.
ARG HELM_VERSION has no default. When the project Makefile drives the build it always passes --build-arg HELM_VERSION=$(HELM_VERSION), so CI is fine. A direct docker build -f docker/Dockerfile.alluxioruntime . is not: ${HELM_VERSION} expands to empty, then make download-helm HELM_VERSION= overrides the Makefile's HELM_VERSION ?= v3.19.5 with the empty string, and the curl URL ends up as .../releases/download//helm--linux-amd64.tar.gz. Adding ARG HELM_VERSION=v3.19.5 here (and in the runtime stage that does the COPY --from=builder .../bin/helm/${HELM_VERSION}/...) would keep the Makefile as the single source of truth and let direct builds work. Same fix applies to all nine Dockerfiles in this PR.
| FROM golang:1.24.12-bookworm@sha256:1c64c586e1cf9dc4c394c5896ec574659c792a0840f4fa0eb54a88de146e978b as builder | ||
|
|
||
| WORKDIR /go/src/github.com/fluid-cloudnative/fluid | ||
| COPY Makefile Makefile |
There was a problem hiding this comment.
Putting COPY Makefile Makefile ahead of COPY . . is the right idea for caching the helm download, but the Makefile is one of the more frequently edited files in this repo, so this layer will invalidate often and re-download helm anyway. Two cheap alternatives: extract just the helm-download logic into a small hack/download-helm.sh and copy that instead of the whole Makefile, or keep it as is and add a comment so the next person doesn't try to 'optimize' by moving it. Same observation applies to all nine Dockerfiles.
|
|
||
| .PHONY: docker-build-all | ||
| docker-build-all: pre-setup ${DOCKER_BUILD} | ||
| docker-build-all: pre-setup download-helm ${DOCKER_BUILD} |
There was a problem hiding this comment.
With host-side download-helm already gating docker-build-all, the in-builder make download-helm in each Dockerfile fires on cold caches up to nine times per matrix run. Not a blocker, but if more runtime images are added it scales linearly. Worth a follow-up to factor a shared builder base image (or a tiny pre-built helm-fetch stage) so the download happens once per build.
…lt HELM_VERSION Move helm download logic from Makefile into hack/download-helm.sh so Dockerfiles COPY only the small script instead of the entire Makefile, improving layer cache stability. Add ARG HELM_VERSION=v3.19.5 default in both builder and runtime stages so direct `docker build` without --build-arg works correctly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
cheyang
force-pushed
the
remove_helm_linux_binary
branch
from
a06cce3 to
26aa8aa
Compare
| #!/bin/bash | ||
| set -euo pipefail | ||
|
|
||
| HELM_VERSION="${1:-v3.19.5}" |
There was a problem hiding this comment.
Nice cleanup — extracting this into a script and adding a default ARG HELM_VERSION=v3.19.5 in the Dockerfiles addresses the earlier feedback well, and the layer cache is much tighter now that only the script is copied. Heads-up that the default version string is now duplicated in three places (Makefile HELM_VERSION ?= v3.19.5, this script, and every Dockerfile builder/runtime stage). Not a blocker for this PR, but a follow-up to source it from a single place would make future helm bumps less error-prone.
|
|
||
| .PHONY: docker-build-all | ||
| docker-build-all: pre-setup ${DOCKER_BUILD} | ||
| docker-build-all: pre-setup download-helm ${DOCKER_BUILD} |
|
|
||
| .PHONY: docker-buildx-all-push | ||
| docker-buildx-all-push: pre-setup ${DOCKER_BUILDX_PUSH} | ||
| docker-buildx-all-push: pre-setup download-helm ${DOCKER_BUILDX_PUSH} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
| ARG HELM_VERSION=v3.19.5 | ||
| RUN hack/download-helm.sh ${HELM_VERSION} |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
|
Remove pre-committed helm ELF binaries (helm-linux-amd64, helm-linux-arm64) from the repository to resolve Scorecard Binary-Artifacts alerts. Helm binaries are now downloaded at build time via
make download-helmin each Dockerfile's builder stage.Ⅰ. Describe what this PR does
Ⅱ. 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
Ⅴ. Special notes for reviews