chore(build): move deps buold codes to onghorn/dep-versions #851
Conversation
WalkthroughThe changes update the CI workflow and associated scripts to dynamically determine a dependency version branch for Docker image builds on both AMD64 and ARM64 architectures. A new step in the GitHub Actions workflow calls a Bash script that retrieves the branch based on a local version file. The Dockerfile and package script now use this branch information to fetch dependency configuration from a remote repository. Additionally, new functions for version conversion and branch retrieval have been added to streamline dependency version management. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions Workflow
participant Script as Build Script (build.sh / package script)
participant File as Local "version" File
participant Remote as Remote Repo (versions.json)
participant Docker as Docker Build Process
CI->>Script: Execute "Get Deps-Version Branch" step
Script->>File: Check for "version" file
File-->>Script: Return version value
Script->>Script: Convert version (convert_version_to_major_minor_x)
Script->>Remote: Attempt to download versions.json using branch value
alt Download succeeds
Remote-->>Script: Return versions.json
Script-->>CI: Echo derived branch value
else Download fails
Script-->>CI: Return "main" as branch value
end
CI->>Docker: Pass BRANCH in build arguments for Docker image
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
scripts/package (1)
51-68: Improve error handling and file path in get_branch function.The function has a few areas for improvement:
- The wget command downloads to /versions.json which might require root permissions
- The exit code check can be simplified
Apply this diff to improve the implementation:
function get_branch() { local version_file="version" if [[ ! -f $version_file ]]; then echo "Error: Version file '$version_file' not found." exit 1 fi local version=$(cat "$version_file") local branch=$(convert_version_to_major_minor_x "$version") # Fetch versions.json from the appropriate branch, fallback to main - wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O /versions.json - if [ $? -eq 0 ]; then + if wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O versions.json; then echo "${branch}" else echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 63-63: scripts/package#L63
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181).github/workflows/scripts/build.sh (1)
12-29: Improve variable declarations and error handling in get_branch function.The function has a few areas for improvement:
- Variable declarations should be separate from assignments to avoid masking return values
- The exit code check can be simplified
Apply this diff to improve the implementation:
function get_branch() { local version_file="version" if [[ ! -f $version_file ]]; then echo "Error: Version file '$version_file' not found." exit 1 fi - local version=$(cat "$version_file") - local branch=$(convert_version_to_major_minor_x "$version") + local version branch + version=$(cat "$version_file") + branch=$(convert_version_to_major_minor_x "$version") # Fetch versions.json from the appropriate branch, fallback to main - wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O versions.json - if [ $? -eq 0 ]; then + if wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O versions.json; then echo "${branch}" else echo "main" fi }🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 GitHub Check: CodeFactor
[notice] 24-24: .github/workflows/scripts/build.sh#L24
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)package/Dockerfile (1)
28-30: Improve wget command in grpc_health_probe installation.Add progress indicator or use quiet mode for better output control.
Apply this diff:
-RUN export GRPC_HEALTH_PROBE_DOWNLOAD_URL=$(wget -qO- https://api.github.com/repos/grpc-ecosystem/grpc-health-probe/releases/latest | jq -r '.assets[] | select(.name | test("linux.*'"${ARCH}"'"; "i")) | .browser_download_url') && \ +RUN export GRPC_HEALTH_PROBE_DOWNLOAD_URL=$(wget --progress=dot:giga -O- https://api.github.com/repos/grpc-ecosystem/grpc-health-probe/releases/latest | jq -r '.assets[] | select(.name | test("linux.*'"${ARCH}"'"; "i")) | .browser_download_url') && \ wget ${GRPC_HEALTH_PROBE_DOWNLOAD_URL} -O /usr/local/bin/grpc_health_probe && \ chmod +x /usr/local/bin/grpc_health_probe🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 28-28: package/Dockerfile#L28
Avoid use of wget without progress bar. Usewget --progress=dot:giga <url>. Or consider using-qor-nv(shorthands for--quietor--no-verbose). (DL3047)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(6 hunks).github/workflows/scripts/build.sh(1 hunks)package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/workflows/scripts/build.sh
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 GitHub Check: CodeFactor
.github/workflows/scripts/build.sh
[notice] 24-24: .github/workflows/scripts/build.sh#L24
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
package/Dockerfile
[notice] 28-28: package/Dockerfile#L28
Avoid use of wget without progress bar. Use wget --progress=dot:giga <url>. Or consider using -q or -nv (shorthands for --quiet or --no-verbose). (DL3047)
scripts/package
[notice] 63-63: scripts/package#L63
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build AMD64 binaries
- GitHub Check: Build ARM64 binaries
- GitHub Check: Summary
🔇 Additional comments (4)
scripts/package (1)
42-49: LGTM!The version conversion function is well-implemented with proper error handling.
.github/workflows/scripts/build.sh (1)
3-10: LGTM!The version conversion function is well-implemented with proper error handling.
package/Dockerfile (1)
19-20: LGTM!The dependency version management is properly set up.
.github/workflows/build.yml (1)
65-70: LGTM!The workflow properly integrates the dependency version management for both AMD64 and ARM64 builds.
Also applies to: 139-144
- move deps buold codes to onghorn/dep-versions - update workflows/build.yml Longhorn 10208 Signed-off-by: Derek Su <[email protected]>
derekbit
force-pushed
the
update-build-yaml
branch
from
2c2658c to
6bc3731
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/package (1)
61-67: Improve error handling in get_branch function.
- Use direct command status check instead of
$?- Consider using a temporary file or cleaning up the downloaded file
- wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O /versions.json - if [ $? -eq 0 ]; then + if wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json" -O versions.json.tmp; then + rm -f versions.json.tmp echo "${branch}" else + rm -f versions.json.tmp echo "main" fi🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 63-63: scripts/package#L63
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)package/Dockerfile (2)
23-78: Optimize build script execution pattern.The current implementation:
- Uses empty override variables consistently
- Repeats the same export pattern multiple times
Consider creating a helper function to reduce repetition:
+RUN echo '#!/bin/bash +build_component() { + local script_name="$1" + local arch="$2" + export REPO_OVERRIDE="" + export COMMIT_ID_OVERRIDE="" + if [ -n "$arch" ]; then + bash "/usr/src/dep-versions/scripts/$script_name" "$REPO_OVERRIDE" "$COMMIT_ID_OVERRIDE" "$arch" + else + bash "/usr/src/dep-versions/scripts/$script_name" "$REPO_OVERRIDE" "$COMMIT_ID_OVERRIDE" + fi +}' > /usr/local/bin/build-helper.sh && chmod +x /usr/local/bin/build-helper.sh -RUN export REPO_OVERRIDE="" && \ - export COMMIT_ID_OVERRIDE="" && \ - bash /usr/src/dep-versions/scripts/build-go-spdk-helper.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}" +RUN build_component build-go-spdk-helper.sh🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 28-28: package/Dockerfile#L28
Avoid use of wget without progress bar. Usewget --progress=dot:giga <url>. Or consider using-qor-nv(shorthands for--quietor--no-verbose). (DL3047)
52-53: Avoid redundant downloads in multi-stage builds.The versions.json file and dep-versions repository are downloaded multiple times in different stages.
Consider copying these files between stages:
+FROM gobuilder as deps +RUN wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${BRANCH}/versions.json" -O /versions.json && \ + git clone https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions FROM registry.suse.com/bci/bci-base:15.6 AS cbuilder -RUN wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/${BRANCH}/versions.json" -O /versions.json -RUN git clone https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions +COPY --from=deps /versions.json /versions.json +COPY --from=deps /usr/src/dep-versions /usr/src/dep-versions
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml(6 hunks).github/workflows/scripts/build.sh(1 hunks)package/Dockerfile(3 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 63-63: scripts/package#L63
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
package/Dockerfile
[notice] 28-28: package/Dockerfile#L28
Avoid use of wget without progress bar. Use wget --progress=dot:giga <url>. Or consider using -q or -nv (shorthands for --quiet or --no-verbose). (DL3047)
.github/workflows/scripts/build.sh
[notice] 24-24: .github/workflows/scripts/build.sh#L24
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
🪛 Shellcheck (0.10.0)
.github/workflows/scripts/build.sh
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 20-20: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build AMD64 binaries
- GitHub Check: Build ARM64 binaries
- GitHub Check: Summary
🔇 Additional comments (1)
package/Dockerfile (1)
19-20: Consider security implications of downloading and executing scripts.The implementation downloads and executes scripts from a remote repository. While this improves maintainability, it introduces security considerations:
- No checksum verification for downloaded content
- No fallback if the remote repository is unavailable
Consider implementing checksum verification:
|
Waiting for https://github.com/longhorn/dep-versions/pulls |
Which issue(s) this PR fixes:
Issue longhorn/longhorn#10208
What this PR does / why we need it:
Special notes for your reviewer:
Additional documentation or context