build: pin runtime chart version to ensure consistency #5370
Conversation
- Add a script to inject the specified Fluid version into all charts - Ensure all charts (except Fluid control-plane and library charts) have the correct appVersion - This change improves consistency and traceability of Fluid versions across charts Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
Skipping CI for Draft Pull Request. |
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5370 +/- ##
==========================================
- Coverage 57.58% 57.56% -0.02%
==========================================
Files 446 447 +1
Lines 30995 31013 +18
==========================================
+ Hits 17849 17854 +5
- Misses 11534 11545 +11
- Partials 1612 1614 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
…ure git tree state is clean Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
/test fluid-e2e |
|
/retest |
|
/gemini review |
![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 introduces a mechanism to pin the appVersion in runtime Helm charts to ensure version consistency across the Fluid ecosystem. This is achieved by adding a new script, hack/helm/pin_runtime_chart_version.sh, which is executed during the Docker image build process. The Makefile and various Dockerfiles are updated to support this new step. The overall approach is sound and effectively addresses the goal of version consistency. My review focuses on improving the newly added shell script for better robustness and clarity. I've provided two suggestions for hack/helm/pin_runtime_chart_version.sh: one to correct a misleading comment and another to refactor the file processing logic to be more robust and idiomatic.
| charts=$(find charts | grep Chart.yaml | grep -v "charts/fluid/\|charts/library" | xargs -I _ dirname _) | ||
|
|
||
| for chart in $charts; do | ||
| if [[ -f "$chart/Chart.yaml" ]]; then | ||
| echo "Processing $chart/Chart.yaml." | ||
| if [[ $(grep -c "appVersion:" $chart/Chart.yaml) == 0 ]]; then | ||
| echo "appVersion: $fluid_version" >> $chart/Chart.yaml | ||
| else | ||
| sed -i 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
The current implementation for finding and processing charts can be simplified and made more robust.
find charts | grep Chart.yamlis less precise thanfind charts -name Chart.yamland can lead to false positives.- The
for chart in $chartsloop is not safe for paths containing spaces due to word splitting. - The logic can be simplified by operating on the
Chart.yamlfile paths directly, removing the need forxargs dirnameand the redundantif [[ -f ... ]]check.
A while read loop is a standard and safer way to process file paths from find.
| charts=$(find charts | grep Chart.yaml | grep -v "charts/fluid/\|charts/library" | xargs -I _ dirname _) | |
| for chart in $charts; do | |
| if [[ -f "$chart/Chart.yaml" ]]; then | |
| echo "Processing $chart/Chart.yaml." | |
| if [[ $(grep -c "appVersion:" $chart/Chart.yaml) == 0 ]]; then | |
| echo "appVersion: $fluid_version" >> $chart/Chart.yaml | |
| else | |
| sed -i 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml | |
| fi | |
| fi | |
| done | |
| find charts -name Chart.yaml | grep -v -e "charts/fluid/" -e "charts/library" | while read -r chart_file; do | |
| echo "Processing $chart_file." | |
| if [[ $(grep -c "appVersion:" "$chart_file") -eq 0 ]]; then | |
| echo "appVersion: $fluid_version" >> "$chart_file" | |
| else | |
| sed -i 's/appVersion:.*/appVersion: '"$fluid_version"'/' "$chart_file" | |
| fi | |
| done |
There was a problem hiding this comment.
Pull request overview
This PR introduces a build-time mechanism to ensure consistent Fluid version tracking across runtime Helm charts by injecting the appVersion field into Chart.yaml files during Docker image builds.
Key Changes:
- Adds a new shell script (
pin_runtime_chart_version.sh) that updates theappVersionfield in Helm charts - Modifies all runtime controller Dockerfiles to execute the version pinning script during the build stage
- Updates the Makefile to pass the
FLUID_VERSIONbuild argument to all Docker builds
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| hack/helm/pin_runtime_chart_version.sh | New script that updates appVersion in Chart.yaml files for runtime charts |
| docker/Dockerfile.vineyardruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.thinruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.juicefsruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.jindoruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.goosefsruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.efcruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.dataset | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.application | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| docker/Dockerfile.alluxioruntime | Adds FLUID_VERSION arg, executes pinning script, and copies modified charts from builder stage |
| Makefile | Centralizes build args including FLUID_VERSION for consistent propagation across all Docker builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ $(grep -c "appVersion:" $chart/Chart.yaml) == 0 ]]; then | ||
| echo "appVersion: $fluid_version" >> $chart/Chart.yaml | ||
| else | ||
| sed -i 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml |
There was a problem hiding this comment.
The sed -i command is missing the backup extension argument that's used in the existing inject_library_chart.sh script (line 24 uses sed -i ""). On macOS, sed -i requires an extension argument (even if empty like ""), while on Linux it's optional. For consistency with the existing script and cross-platform compatibility, this should be sed -i "" 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml
| sed -i 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml | |
| sed -i "" 's/appVersion:.*/appVersion: '"$fluid_version"'/' $chart/Chart.yaml |
| exit 1 | ||
| fi | ||
|
|
||
| # All charts should be injected except for Fluid control-plane chart and library chart itself. |
There was a problem hiding this comment.
The comment says "All charts should be injected" but this script is about pinning versions, not injecting. It should say something like "Process all charts except for Fluid control-plane chart and library chart itself."
| # All charts should be injected except for Fluid control-plane chart and library chart itself. | |
| # Process all charts except for Fluid control-plane chart and library chart itself. |
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
/test fluid-e2e |
- Add a try-except block to handle flaky potential exceptions when checking DataLoad status Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
/retest |
|
|
/test fluid-e2e |
|
@TrafalgarZZZ: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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/test-infra repository. I understand the commands that are listed here. |
2 participants
Ⅰ. 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