import helm-bin from local for cacheruntime#5795
import helm-bin from local for cacheruntime#5795fluid-e2e-bot[bot] merged 1 commit intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
![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 Alpine base image version and modifies the Dockerfile to copy the Helm binary from the build context instead of downloading it. Feedback indicates that the specified Alpine version 3.23.3 is invalid and should likely be 3.21.3. Furthermore, the change to the Helm binary source requires an update to the Makefile to ensure the binary is available during the build process, preventing potential failures in clean environments.
| # alpine:3.23.3 | ||
| FROM alpine:3.23.3@sha256:25109184c71bdad752c8312a8623239686a9a2071e8825f20acb8f2198c3f659 |
There was a problem hiding this comment.
The Alpine version 3.23.3 is invalid as Alpine Linux has not released a version 3.23 (the current latest stable is 3.21). This will cause the image build to fail during the base image pull. Based on the SHA provided, it is highly likely that 3.21.3 was intended.
# alpine:3.21.3
FROM alpine:3.21.3@sha256:25109184c71bdad752c8312a8623239686a9a2071e8825f20acb8f2198c3f659
| chmod u+x /usr/local/bin/ddc-helm && \ | ||
| rm -f helm-${HELM_VERSION}-linux-${TARGETARCH}.tar.gz && \ | ||
| rm -rf linux-${TARGETARCH} | ||
| 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.
This change makes the Docker build dependent on the Helm binary being present in the local bin/ directory of the build context. However, the Makefile does not currently ensure that make download-helm is executed before docker-build-cacheruntime-controller. This will cause builds to fail in clean environments or CI pipelines where the binary hasn't been manually downloaded. To fix this, you should update the Makefile to include download-helm as a dependency for the Docker build targets.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5795 +/- ##
=======================================
Coverage 58.46% 58.46%
=======================================
Files 473 473
Lines 32222 32222
=======================================
Hits 18839 18839
Misses 11836 11836
Partials 1547 1547 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Good change — aligning cacheruntime Dockerfile with the same COPY-from-builder pattern used by all other runtime Dockerfiles (alluxio, dataset, efc, etc.). A few observations: Positive:
Note (not blocking, pre-existing): This looks ready to merge. |
|
[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 participants
Ⅰ. Describe what this PR does
cacheruntime docker build file uses helm-bin from local.
Ⅱ. 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