feat(fluid-helm): import helm from local#5752
feat(fluid-helm): import helm from local#5752cheyang merged 1 commit intofluid-cloudnative:masterfrom
Conversation
![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 refactors the Helm binary acquisition process by introducing a download-helm target in the Makefile and updating several Dockerfiles to copy pre-downloaded binaries from the builder stage. Reviewer feedback suggests maintaining the Git ignore rule for the binary directory to prevent repository bloat, parameterizing architectures in the Makefile for flexibility, improving the reliability of the download pipe, and using relative paths in Dockerfile COPY instructions.
| *.dylib | ||
| *.jar | ||
| bin | ||
| !bin/helm/ |
There was a problem hiding this comment.
Un-ignoring bin/helm/ in .gitignore might lead to large binaries being accidentally committed to the repository. If the goal is to include these binaries in the Docker build context, it is better to keep them ignored in Git and ensure they are not excluded in .dockerignore. If you intend to track these binaries in Git, please consider the impact on repository size.
| .PHONY: download-helm | ||
| download-helm: | ||
| mkdir -p $(HELM_BINARY_DIR) | ||
| @for arch in amd64 arm64; do \ |
There was a problem hiding this comment.
The architectures are hardcoded to amd64 and arm64. This prevents building for other architectures that might be supported by the project (e.g., ppc64le, s390x). Consider using a variable like SUPPORTED_ARCHS ?= amd64 arm64 to make this more flexible.
@for arch in $(SUPPORTED_ARCHS); do \
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz \ | ||
| | tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) linux-$${arch}/helm; \ |
There was a problem hiding this comment.
When piping curl to tar, if curl fails (e.g., due to a 404 or network issue), tar might still attempt to process the output or fail with a confusing error. It's safer to download the file first or use a shell that supports set -o pipefail to ensure the command fails if any part of the pipe fails.
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
Using an absolute path for the COPY source is brittle and depends on the specific directory structure inside the builder stage. Since the WORKDIR is already set to /go/src/github.com/fluid-cloudnative/fluid, you can use a relative path.
COPY --from=builder bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5752 +/- ##
=======================================
Coverage 61.22% 61.22%
=======================================
Files 444 444
Lines 30557 30557
=======================================
Hits 18710 18710
Misses 10307 10307
Partials 1540 1540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to stop downloading Helm during Docker image builds by sourcing a Helm binary from a local path in the repository/build context.
Changes:
- Added a
make download-helmtarget to download Helm binaries intobin/helm/<version>/. - Updated multiple runtime/controller Dockerfiles to
COPYHelm frombin/helm/${HELM_VERSION}/...instead of downloading it at image build time. - Updated
.gitignoreto (partially) unignorebin/helm/.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds download-helm target and a versioned Helm binary directory under bin/helm/. |
| docker/Dockerfile.vineyardruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.thinruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.juicefsruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.jindoruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.goosefsruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.efcruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.dataset | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.application | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| docker/Dockerfile.alluxioruntime | Switches to copying ddc-helm from the builder stage’s bin/helm/... path. |
| .gitignore | Attempts to unignore bin/helm/ under an otherwise-ignored bin/ directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ##@ Helm Binary | ||
|
|
||
| HELM_BINARY_DIR := $(shell pwd)/bin/helm/$(HELM_VERSION) | ||
|
|
||
| # Download helm binaries for linux/amd64 and linux/arm64 to bin/helm/<version>/ | ||
| # Run this target when upgrading HELM_VERSION or on a fresh checkout. | ||
| .PHONY: download-helm | ||
| download-helm: | ||
| mkdir -p $(HELM_BINARY_DIR) |
There was a problem hiding this comment.
The Dockerfiles now expect a pre-existing Helm binary at bin/helm/$(HELM_VERSION)/helm-linux-, but this Makefile doesn't hook download-helm into any docker-build-* / docker-buildx-* targets (and the Dockerfile builder stages don't run it either). As-is, docker builds will fail unless callers manually populate that path; consider wiring download-helm into the docker build flow (either run it in the Dockerfile builder stage or add it as a prerequisite for the Makefile docker targets and ensure CI uses those targets).
| @for arch in amd64 arm64; do \ | ||
| target=$(HELM_BINARY_DIR)/helm-linux-$${arch}; \ | ||
| if [ ! -f "$${target}" ]; then \ | ||
| echo "Downloading helm $(HELM_VERSION) linux/$${arch} ..."; \ | ||
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz \ | ||
| | tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) linux-$${arch}/helm; \ | ||
| mv $(HELM_BINARY_DIR)/helm $${target}; \ | ||
| chmod +x $${target}; \ | ||
| else \ | ||
| echo "helm $(HELM_VERSION) linux/$${arch} already exists, skipping."; \ | ||
| fi; \ | ||
| done |
There was a problem hiding this comment.
download-helm's for loop doesn't fail fast: if one architecture download/extract fails, the loop can continue and still exit 0 if a later iteration succeeds. Consider forcing the recipe to exit on the first failure (e.g., enable set -e within the loop and/or explicitly track/return a non-zero status when any arch fails).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make dataset-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make application-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make alluxioruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | |
| RUN chmod u+x /usr/local/bin/ddc-helm | |
| RUN curl -fsSL "https://get.helm.sh/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz" \ | |
| | tar -xzO "linux-${TARGETARCH}/helm" > /usr/local/bin/ddc-helm && \ | |
| chmod u+x /usr/local/bin/ddc-helm |
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make thinruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make juicefsruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make jindoruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make goosefsruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | |
| RUN chmod u+x /usr/local/bin/ddc-helm | |
| RUN curl -sSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-${TARGETARCH}.tar.gz" -o /tmp/helm.tgz && \ | |
| tar -xzf /tmp/helm.tgz -C /tmp && \ | |
| mv "/tmp/linux-${TARGETARCH}/helm" /usr/local/bin/ddc-helm && \ | |
| chmod u+x /usr/local/bin/ddc-helm && \ | |
| rm -rf /tmp/helm.tgz "/tmp/linux-${TARGETARCH}" |
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
This COPY --from=builder .../bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} assumes the Helm binary already exists in the builder stage filesystem, but the builder stage only runs make efcruntime-controller-build and doesn't create/download that file. Unless bin/helm/... is provided in the build context ahead of time, docker build will fail here; consider adding a builder-stage step to populate it (e.g., run make download-helm and ensure curl/tar are available) or revert to downloading Helm during the image build.
Signed-off-by: 玖宇 <guotongyu.gty@alibaba-inc.com>
Syspretor
force-pushed
the
feature/import-helm-from-local
branch
from
739b264 to
c385872
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @for arch in amd64 arm64; do \ | ||
| target=$(HELM_BINARY_DIR)/helm-linux-$${arch}; \ | ||
| if [ ! -f "$${target}" ]; then \ | ||
| echo "Downloading helm $(HELM_VERSION) linux/$${arch} ..."; \ | ||
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz \ | ||
| | tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) linux-$${arch}/helm; \ |
There was a problem hiding this comment.
download-helm uses a curl | tar pipeline, but without pipefail the target can succeed even if the download fails (the pipeline exit status is typically the tar exit code). Consider downloading to a temp file first (or run the recipe under bash with set -o pipefail) so network/HTTP failures reliably fail the Make target.
| @for arch in amd64 arm64; do \ | |
| target=$(HELM_BINARY_DIR)/helm-linux-$${arch}; \ | |
| if [ ! -f "$${target}" ]; then \ | |
| echo "Downloading helm $(HELM_VERSION) linux/$${arch} ..."; \ | |
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz \ | |
| | tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) linux-$${arch}/helm; \ | |
| @set -e; \ | |
| for arch in amd64 arm64; do \ | |
| target=$(HELM_BINARY_DIR)/helm-linux-$${arch}; \ | |
| if [ ! -f "$${target}" ]; then \ | |
| echo "Downloading helm $(HELM_VERSION) linux/$${arch} ..."; \ | |
| tmp_tar="$(HELM_BINARY_DIR)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz"; \ | |
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz -o "$${tmp_tar}"; \ | |
| tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) -f "$${tmp_tar}" linux-$${arch}/helm; \ | |
| rm -f "$${tmp_tar}"; \ |
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail at build time with a missing file. Either enforce a single HELM_VERSION across all builds, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., via make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| ARG TARGETARCH | ||
| ARG HELM_VERSION | ||
| RUN wget -O helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz https://github.com/fluid-cloudnative/helm/releases/download/${HELM_VERSION}/helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| tar -xvf helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| mv linux-${TARGETARCH}/helm /usr/local/bin/ddc-helm && \ | ||
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The repo only contains helm binaries under bin/helm/v3.19.5/, but this Dockerfile copies from bin/helm/${HELM_VERSION}. Any build that passes a different HELM_VERSION will fail with a missing file. Either align build args to the vendored version, or add a builder-stage step that populates bin/helm/${HELM_VERSION} (e.g., make download-helm).
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f ${HELM_VERSION}-linux-${TARGETARCH}.tar.gz | ||
| COPY --from=builder /go/src/github.com/fluid-cloudnative/fluid/bin/helm/${HELM_VERSION}/helm-linux-${TARGETARCH} /usr/local/bin/ddc-helm | ||
| RUN chmod u+x /usr/local/bin/ddc-helm |
There was a problem hiding this comment.
The Dockerfiles now copy Helm from bin/helm/${HELM_VERSION}/..., but this PR only vendors binaries under bin/helm/v3.19.5/. That means HELM_VERSION still looks configurable from the build interface, while in practice any non-vendored version will fail at build time with a missing file. Before this change, overriding HELM_VERSION still worked because the image downloaded the requested version during build. With the new layout, that behavior is effectively narrowed to the single vendored version unless download-helm is wired into the build flow. I think this should be addressed before merge: either make the build flow populate bin/helm/${HELM_VERSION} reliably, or explicitly lock the build interface to the vendored Helm version.
| if [ ! -f "$${target}" ]; then \ | ||
| echo "Downloading helm $(HELM_VERSION) linux/$${arch} ..."; \ | ||
| curl -fsSL https://github.com/fluid-cloudnative/helm/releases/download/$(HELM_VERSION)/helm-$(HELM_VERSION)-linux-$${arch}.tar.gz \ | ||
| | tar -xz --strip-components=1 -C $(HELM_BINARY_DIR) linux-$${arch}/helm; \ |
There was a problem hiding this comment.
download-helm would be more robust if it failed hard on download or extract errors instead of relying on a curl | tar pipeline. Downloading to a temporary file first would make failure handling and debugging much clearer.
| *.dylib | ||
| *.jar | ||
| bin | ||
| /bin/* |
There was a problem hiding this comment.
This change also formalizes committing large Helm binaries into the repo history. If that is intentional, it may be worth calling out explicitly in the PR description because it changes the repository size and maintenance trade-off, not just the Docker build implementation.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 similar comments
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
(fluid-helm): import helm-bin from local
Ⅰ. 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