chore: versions of dependent packages are centrally managed in longhorn/dep-versions #445
Conversation
WalkthroughThe updates modify the Dockerfile and accompanying build scripts. In the Dockerfile, new arguments ( Changes
Sequence Diagram(s)sequenceDiagram
participant D as Docker Build Process
participant G as GitHub (dep-versions)
participant S as Build Script
Note over D: Set ARG SRC_BRANCH=main
D->>G: Clone repository using SRC_BRANCH
G-->>D: Provide repository files
D->>S: Run build-nfs-ganesha.sh
sequenceDiagram
participant S as scripts/package
participant U as System (uname)
participant G as GitHub (versions.json)
S->>U: Execute uname -m to detect architecture
U-->>S: Return ARCH (arm64/amd64)
S->>S: Run get_branch() to read version file
S->>G: Attempt to download versions.json (using branch from version)
alt Fetch Fails
S-->>S: Default SRC_BRANCH to main
else Fetch Succeeds
S-->>S: Set SRC_BRANCH based on version
end
S->>BuildSystem: Trigger build with ARCH and SRC_BRANCH arguments
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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
|
derekbit
force-pushed
the
issue-10208
branch
3 times, most recently
from
214e7f3 to
b9785dc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/package (2)
36-43: Enhance version format validation.The version format validation could be more strict and informative.
Consider this improved implementation:
function convert_version_to_major_minor_x() { local version="$1" - if [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then + if [[ -z "$version" ]]; then + echo "Error: Version cannot be empty" + return 1 + elif [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then echo "v${BASH_REMATCH[1]}.${BASH_REMATCH[2]}.x" else - echo "Invalid version format: $version" + echo "Error: Invalid version format: $version. Expected format: vX.Y.Z" + return 1 fi }
71-76: Use modern Docker buildx syntax.The build command uses legacy syntax.
Consider using modern Docker buildx syntax:
-buildx build \ +docker buildx build \ --load \ --build-arg ARCH="${ARCH}" \ --build-arg BRANCH="${BRANCH}" \ + --platform "linux/${ARCH}" \ -t "${IMAGE}" \ -f package/Dockerfile .package/Dockerfile (1)
22-27: Improve build script error handling.The build script execution could be more robust.
Consider this improved implementation:
-RUN git clone https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions +RUN git clone --depth 1 https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions -# Build nfs-ganesha -RUN export REPO_OVERRIDE="" && \ - export COMMIT_ID_OVERRIDE="" && \ - bash /usr/src/dep-versions/scripts/build-nfs-ganesha.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}" +# Build nfs-ganesha with error handling +RUN set -e && \ + export REPO_OVERRIDE="" && \ + export COMMIT_ID_OVERRIDE="" && \ + if ! bash /usr/src/dep-versions/scripts/build-nfs-ganesha.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}"; then \ + echo "Error: NFS-Ganesha build failed" && exit 1; \ + fi
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 57-57: scripts/package#L57
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 29-29: scripts/package#L29
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 46-46: package/Dockerfile#L46
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (3)
package/Dockerfile (3)
5-5: Consider validating BRANCH argument.The BRANCH argument should be validated to prevent potential security issues.
Add validation for the BRANCH argument:
ARG BRANCH=main +RUN if [[ ! "$BRANCH" =~ ^[a-zA-Z0-9._-]+$ ]]; then \ + echo "Error: Invalid BRANCH format" && exit 1; \ + fi
17-20: 🛠️ Refactor suggestionFix package versioning and cleanup.
Package versions should be pinned for reproducible builds.
Let's check the latest stable versions of these packages:
RUN zypper -n install autoconf bison curl cmake doxygen make git gcc14 gcc14-c++ flex Mesa-libGL-devel libdbus-1-3 \ nfsidmap-devel liburcu-devel libblkid-devel e2fsprogs e2fsprogs-devel xfsprogs xfsprogs-devel \ - tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ + tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ rm -rf /var/cache/zypp/*
46-48: 🛠️ Refactor suggestionPin package versions for reproducibility.
Package versions should be pinned for reproducible builds.
Let's check the latest stable versions of these packages:
Additionally, clean up after package installation:
-RUN zypper -n install rpcbind hostname libblkid1 liburcu6 libjson-c* dbus-1-x11 dbus-1 nfsidmap-devel \ - nfs-kernel-server nfs-client nfs4-acl-tools xfsprogs e2fsprogs awk && \ - rm -rf /var/cache/zypp/* +RUN zypper -n install \ + rpcbind \ + hostname \ + libblkid1 \ + liburcu6 \ + libjson-c3 \ + dbus-1-x11 \ + dbus-1 \ + nfsidmap-devel \ + nfs-kernel-server \ + nfs-client \ + nfs4-acl-tools \ + xfsprogs \ + e2fsprogs \ + awk && \ + zypper clean && \ + rm -rf /var/cache/zypp/*🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 46-46: package/Dockerfile#L46
Specify version withzypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper cleanmissing after zypper use. (DL3036)
derekbit
force-pushed
the
issue-10208
branch
from
b9785dc to
f5e27d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/package (2)
16-29: 🛠️ Refactor suggestionImprove architecture detection and API version retrieval.
The architecture detection and API version retrieval logic needs improvement:
- Missing error handling for unsupported architectures (e.g., x86)
- Using legacy backticks instead of $() notation
- No error handling for API version retrieval failure
Consider this improved implementation:
-#APIVERSION=`./bin/longhorn-share-manager version --client-only|jq ".clientVersion.apiVersion"` - -# Try bin/longhorn-share-manager-amd64 and bin/longhorn-share-manager-arm64 doesn't have "Exec format error" -# if it is not "Exec format error", use it to get the version -#APIVERSION=`./bin/longhorn-share-manager version --client-only|jq ".clientVersion.apiVersion"` - -APIVERSION="" -arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi -APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +# Detect system architecture +arch=$(uname -m) +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac + +# Get API version using architecture-specific binary +if ! APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion"); then + echo "Error: Failed to get API version" + exit 1 +fi🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 29-29: scripts/package#L29
Use $(...) notation instead of legacy backticks.... (SC2006)
45-62: 🛠️ Refactor suggestionImprove error handling in branch detection.
The branch detection could handle network failures more gracefully.
Consider this improved 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 branch + if ! branch=$(convert_version_to_major_minor_x "$version"); then + echo "Error: Failed to convert version to branch format" + exit 1 + fi # 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 --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 57-57: scripts/package#L57
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
🧹 Nitpick comments (4)
scripts/package (1)
36-43: LGTM with minor suggestions.The version conversion function is well-implemented with proper regex pattern matching. Consider adding return code for error cases.
function convert_version_to_major_minor_x() { local version="$1" if [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then echo "v${BASH_REMATCH[1]}.${BASH_REMATCH[2]}.x" + return 0 else echo "Invalid version format: $version" >&2 + return 1 fi }package/Dockerfile (3)
17-20: Consider pinning package versions.While the package list is comprehensive, consider pinning specific versions for better reproducibility.
Example:
- tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ + tar=2.* gzip=1.* dbus-1-devel=1.* lsb-release=* graphviz-devel=* libnsl-devel=* libcurl-devel=* libjson-c-devel=* libacl-devel=* jq=1.* && \
22-22: Optimize git clone operation.Consider using
--depth 1to create a shallow clone for faster builds since historical commits aren't needed.-RUN git clone https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions +RUN git clone --depth 1 https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions
24-27: Document the purpose of override parameters.Consider adding a comment explaining the purpose of
REPO_OVERRIDEandCOMMIT_ID_OVERRIDEparameters.+# REPO_OVERRIDE and COMMIT_ID_OVERRIDE allow customizing the source repository and commit +# Empty values use defaults from dep-versions repository RUN export REPO_OVERRIDE="" && \ export COMMIT_ID_OVERRIDE="" && \ bash /usr/src/dep-versions/scripts/build-nfs-ganesha.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 57-57: scripts/package#L57
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 29-29: scripts/package#L29
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 46-46: package/Dockerfile#L46
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
scripts/package (1)
70-76: LGTM!The build command is properly updated to include the new BRANCH argument.
package/Dockerfile (1)
5-5: LGTM!The BRANCH argument is properly declared with a sensible default value.
derekbit
force-pushed
the
issue-10208
branch
from
f5e27d4 to
89a6d2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/package (2)
16-29: 🛠️ Refactor suggestionImprove architecture detection and command substitution.
The architecture detection could be more robust and the command substitution syntax needs modernization.
Apply this diff to improve the implementation:
-#APIVERSION=`./bin/longhorn-share-manager version --client-only|jq ".clientVersion.apiVersion"` - -# Try bin/longhorn-share-manager-amd64 and bin/longhorn-share-manager-arm64 doesn't have "Exec format error" -# if it is not "Exec format error", use it to get the version -#APIVERSION=`./bin/longhorn-share-manager version --client-only|jq ".clientVersion.apiVersion"` - -APIVERSION="" -arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi -APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +# Detect system architecture +arch=$(uname -m) +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac + +# Get API version using architecture-specific binary +if ! APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion"); then + echo "Error: Failed to get API version" + exit 1 +fi🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 29-29: scripts/package#L29
Use $(...) notation instead of legacy backticks.... (SC2006)
45-62: 🛠️ Refactor suggestionImprove error handling in branch detection.
The branch detection could handle errors more gracefully and avoid using $?.
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 branch + if ! branch=$(convert_version_to_major_minor_x "$version"); then + echo "Error: Failed to convert version to branch format" + exit 1 + fi # 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 --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 57-57: scripts/package#L57
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
🧹 Nitpick comments (4)
scripts/package (1)
36-43: LGTM! Consider adding input validation.The version conversion function is well-structured with proper error handling.
Consider adding input validation:
function convert_version_to_major_minor_x() { local version="$1" + if [ -z "$version" ]; then + echo "Error: Version argument is required" + return 1 + fi if [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then echo "v${BASH_REMATCH[1]}.${BASH_REMATCH[2]}.x" else echo "Invalid version format: $version" + return 1 fi }package/Dockerfile (3)
17-20: Consider pinning package versions for reproducibility.While the package list is comprehensive, pinning versions would ensure consistent builds.
Example for jq package:
- tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ + tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq=1.6-* && \
22-27: Document the purpose of override variables.The implementation looks good, but please document why REPO_OVERRIDE and COMMIT_ID_OVERRIDE are empty.
Add comments explaining the override variables:
RUN git clone https://github.com/longhorn/dep-versions.git -b ${BRANCH} /usr/src/dep-versions # Build nfs-ganesha +# REPO_OVERRIDE and COMMIT_ID_OVERRIDE are intentionally empty to use defaults from dep-versions RUN export REPO_OVERRIDE="" && \ export COMMIT_ID_OVERRIDE="" && \ bash /usr/src/dep-versions/scripts/build-nfs-ganesha.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}"
46-48: Consider pinning package versions and moving cleanup.While the package installation works, it could be more robust.
Apply these improvements:
-RUN zypper -n install rpcbind hostname libblkid1 liburcu6 libjson-c* dbus-1-x11 dbus-1 nfsidmap-devel \ - nfs-kernel-server nfs-client nfs4-acl-tools xfsprogs e2fsprogs awk && \ - rm -rf /var/cache/zypp/* +RUN zypper -n install \ + rpcbind=1.2.6-* \ + hostname=3.23-* \ + libblkid1=2.37-* \ + liburcu6=0.13.1-* \ + libjson-c5=0.15-* \ + dbus-1-x11=1.12.2-* \ + dbus-1=1.12.2-* \ + nfsidmap-devel=2.5.4-* \ + nfs-kernel-server=2.5.4-* \ + nfs-client=2.5.4-* \ + nfs4-acl-tools=0.3.3-* \ + xfsprogs=5.12.0-* \ + e2fsprogs=1.46.2-* \ + awk=5.0.1-* && \ + zypper clean && \ + rm -rf /var/cache/zypp/*🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 46-46: package/Dockerfile#L46
Specify version withzypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper cleanmissing after zypper use. (DL3036)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
package/Dockerfile
[warning] 46-46: package/Dockerfile#L46
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper clean missing after zypper use. (DL3036)
scripts/package
[notice] 57-57: scripts/package#L57
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 29-29: scripts/package#L29
Use $(...) notation instead of legacy backticks .... (SC2006)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
scripts/package (1)
70-76: LGTM! Build configuration is properly set up.The build command correctly passes ARCH and BRANCH arguments to support the centralized version management.
package/Dockerfile (1)
5-5: LGTM! Branch argument is properly defined.The BRANCH argument aligns with the centralized version management approach.
derekbit
force-pushed
the
issue-10208
branch
2 times, most recently
from
cbd6e9b to
b861a38
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/package (2)
39-56: Improve error handling in branch detection.The branch detection could be more robust:
- Use direct command checking instead of $?
- Add better error handling for wget failures
Apply this diff:
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 branch + if ! branch=$(convert_version_to_major_minor_x "$version"); then + echo "Error: Failed to convert version to branch format" + exit 1 + fi # 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 --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 51-51: scripts/package#L51
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
16-23: Improve architecture detection and command substitution.The architecture detection can be more robust and modern:
- Use
$(...)instead of backticks- Add explicit handling for unsupported architectures
Apply this diff:
APIVERSION="" arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi -APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac +APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq ".clientVersion.apiVersion")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.APIVERSION="" arch=$(uname -m) case "$arch" in "x86_64") ARCH="amd64" ;; "aarch64") ARCH="arm64" ;; *) echo "Error: Unsupported architecture: $arch" exit 1 ;; esac APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq ".clientVersion.apiVersion")🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 23-23: scripts/package#L23
Use $(...) notation instead of legacy backticks.... (SC2006)
🧹 Nitpick comments (1)
package/Dockerfile (1)
17-20: Consider pinning package versions for reproducibility.While the package list is comprehensive, consider pinning versions to ensure consistent builds.
Example for key packages:
- tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ + tar=2.* gzip=1.* dbus-1-devel=1.* lsb-release=* graphviz-devel=* libnsl-devel=* libcurl-devel=7.* libjson-c-devel=* libacl-devel=* jq=1.* && \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 51-51: scripts/package#L51
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 23-23: scripts/package#L23
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 46-46: package/Dockerfile#L46
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (4)
scripts/package (2)
30-37: LGTM! Well-structured version conversion function.The function properly handles version extraction and error cases.
64-70: LGTM! Build configuration properly updated.The build command correctly includes the new ARCH and BRANCH arguments.
package/Dockerfile (2)
22-28: Verify the purpose of empty overrides.The build script is called with empty
REPO_OVERRIDEandCOMMIT_ID_OVERRIDE. Please clarify if this is intentional or if these should be populated with specific values.
46-48: LGTM! Package installation looks good.The required NFS packages are properly included, and cache cleanup is handled correctly.
🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 46-46: package/Dockerfile#L46
Specify version withzypper install -y <package>=<version>. (DL3037)
[warning] 46-46: package/Dockerfile#L46
zypper cleanmissing after zypper use. (DL3036)
derekbit
force-pushed
the
issue-10208
branch
from
b861a38 to
833db7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/package (2)
39-56: Optimize branch detection logic.The function downloads
versions.jsonbut doesn't use it, and uses$?for exit code checking.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 branch + if ! branch=$(convert_version_to_major_minor_x "$version"); then + echo "Error: Failed to convert version to branch format" + exit 1 + fi # 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 --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.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 if ! branch=$(convert_version_to_major_minor_x "$version"); then echo "Error: Failed to convert version to branch format" exit 1 fi # Fetch versions.json from the appropriate branch, fallback to main if wget -q --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/versions.json"; then echo "${branch}" else echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 51-51: scripts/package#L51
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
16-23: Improve architecture detection with explicit validation.The current architecture detection logic could fail silently on unsupported architectures.
Consider this improved implementation:
APIVERSION="" arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.APIVERSION="" arch=$(uname -m) case "$arch" in "x86_64") ARCH="amd64" ;; "aarch64") ARCH="arm64" ;; *) echo "Error: Unsupported architecture: $arch" exit 1 ;; esac APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"`🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 23-23: scripts/package#L23
Use $(...) notation instead of legacy backticks.... (SC2006)
🧹 Nitpick comments (3)
scripts/package (2)
23-23: Use modern command substitution syntax.Replace legacy backticks with
$()for better readability and nesting support.-APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq ".clientVersion.apiVersion")🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 23-23: scripts/package#L23
Use $(...) notation instead of legacy backticks.... (SC2006)
30-37: Enhance error handling in version conversion.While the function handles invalid formats, it should return a non-zero exit code for error cases to maintain consistent error handling throughout the script.
function convert_version_to_major_minor_x() { local version="$1" if [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then echo "v${BASH_REMATCH[1]}.${BASH_REMATCH[2]}.x" + return 0 else echo "Invalid version format: $version" + return 1 fi }package/Dockerfile (1)
52-54: Avoid wildcards in package versions.Using wildcards (
libjson-c*) in package versions can lead to inconsistent builds.- nfs-kernel-server nfs-client nfs4-acl-tools xfsprogs e2fsprogs awk && \ + nfs-kernel-server nfs-client nfs4-acl-tools xfsprogs e2fsprogs awk libjson-c3 && \🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 52-52: package/Dockerfile#L52
Specify version withzypper install -y <package>=<version>. (DL3037)
[warning] 52-52: package/Dockerfile#L52
zypper cleanmissing after zypper use. (DL3036)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 51-51: scripts/package#L51
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 23-23: scripts/package#L23
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 52-52: package/Dockerfile#L52
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 52-52: package/Dockerfile#L52
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (3)
scripts/package (1)
65-72: LGTM!The build command is properly structured with all necessary arguments.
package/Dockerfile (2)
23-28: LGTM!The repository cloning logic properly handles both branch and tag checkouts with appropriate error checking.
18-21: Consider pinning package versions for reproducible builds.While the packages are properly installed and cleaned up, consider pinning versions to ensure reproducible builds.
✅ Verification successful
🌐 Web query:
What is the latest version of jq package for SUSE Linux Enterprise?💡 Result:
The latest upstream version of
jqis 1.7.1, which includes security fixes and new features like improved CLI options and syntax updates[3][6]. However, for SUSE Linux Enterprise distributions:
Distribution Version Latest Available jq Version Source SLE 12 SP1-SP5 1.5-6.1 (2019 release) [SUSE Package Hub][1] SLE 15 1.6-3.x series [OpenSUSE Leap 15.x packages][7][9] Newer Rolling Releases Up to 1.7.x via third-party repos [GitHub releases][3][6] For enterprise environments:
- Official SUSE repos currently provide v1.6 as the latest stable version[7][9]
- Version 1.7.x requires manual installation from source or third-party repositories[3][6]
- Critical security fixes from 1.7.x (like CVE-2023-50246) haven't been backported to older SUSE packages[3][8]
To check your installed version:
jq --versionFor newer features, consider compiling from source using instructions from [jqlang.org/download][6], though this isn't recommended for production systems without proper validation[8].
Citations:
- 1: https://packagehub.suse.com/packages/jq/
- 2: https://software.opensuse.org/download/package?package=jq&project=utilities
- 3: https://github.com/jqlang/jq/releases
- 4: https://software.opensuse.org/download/package?package=jq&project=SUSE%3ASLE-15%3AUpdate
- 5: https://github.com/bobbyiliev/introduction-to-bash-scripting/blob/main/ebook/en/content/018-working-with-json-in-bash-using-jq.md
- 6: https://jqlang.org/download/
- 7: https://rpmfind.net/linux/rpm2html/search.php?query=jq
- 8: Cannot install jq on SUSE SLES 12 SP5 jqlang/jq#2399
- 9: https://software.opensuse.org/package/jq
- 10: https://packagehub.suse.com
Ensure reproducible builds by pinning package versions.
The current Dockerfile installs packages without specifying versions. For example, the installed
jqversion can vary depending on the SUSE environment (official repos typically provide jq 1.6 for SLE 15, while upstream is at 1.7.1). Pinning versions helps to avoid unexpected discrepancies in reproducibility.
- Location:
package/Dockerfile, Lines 18-21
derekbit
force-pushed
the
issue-10208
branch
from
833db7b to
e8cf324
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/package (2)
16-22: 🛠️ Refactor suggestionImprove architecture detection and API version retrieval.
The current implementation has several issues:
- Lacks explicit handling for x86_64 architecture
- Missing error handling for unsupported architectures
- Uses legacy backticks for command substitution
Apply this diff to improve the implementation:
-arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi -APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +# Detect system architecture +arch=$(uname -m) +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac + +# Get API version using architecture-specific binary +if ! APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion"); then + echo "Error: Failed to get API version" + exit 1 +fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Detect system architecture arch=$(uname -m) case "$arch" in "x86_64") ARCH="amd64" ;; "aarch64") ARCH="arm64" ;; *) echo "Error: Unsupported architecture: $arch" exit 1 ;; esac # Get API version using architecture-specific binary if ! APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion"); then echo "Error: Failed to get API version" exit 1 fi🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 22-22: scripts/package#L22
Use $(...) notation instead of legacy backticks.... (SC2006)
38-55: 🛠️ Refactor suggestionImprove branch detection and remove unnecessary download.
Based on past review comments and code analysis:
- The
versions.jsonfile is downloaded but not used- Uses indirect exit code checking ($?)
- Error handling could be improved
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 branch + if ! branch=$(convert_version_to_major_minor_x "$version"); then + echo "Error: Failed to convert version to branch format" + exit 1 + fi # 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 --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.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 if ! branch=$(convert_version_to_major_minor_x "$version"); then echo "Error: Failed to convert version to branch format" exit 1 fi # Fetch versions.json from the appropriate branch, fallback to main if wget -q --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/"; then echo "${branch}" else echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 50-50: scripts/package#L50
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
🧹 Nitpick comments (4)
scripts/package (2)
29-36: Enhance version conversion error handling.The function provides basic error handling but could be improved by:
- Validating input is not empty
- Providing more specific error messages
Apply this diff to improve the implementation:
function convert_version_to_major_minor_x() { local version="$1" + if [ -z "$version" ]; then + echo "Error: Version string is empty" + return 1 + fi if [[ "$version" =~ ^v([0-9]+)\.([0-9]+)\. ]]; then echo "v${BASH_REMATCH[1]}.${BASH_REMATCH[2]}.x" else - echo "Invalid version format: $version" + echo "Error: Invalid version format. Expected format: v<major>.<minor>.<patch>, got: $version" + return 1 fi }
57-58: Add error handling for tag retrieval.The tag retrieval could fail silently if git command fails.
Apply this diff to improve the implementation:
-SRC_BRANCH=$(get_branch) -SRC_TAG=$(git tag --points-at HEAD | head -n 1) +if ! SRC_BRANCH=$(get_branch); then + echo "Error: Failed to determine source branch" + exit 1 +fi + +# Get tag pointing at HEAD, if any +if ! SRC_TAG=$(git tag --points-at HEAD | head -n 1); then + echo "Warning: Failed to retrieve tag information" + SRC_TAG="" +fipackage/Dockerfile (2)
18-21: Consider pinning package versions for reproducible builds.While the implementation works, consider pinning package versions to ensure reproducible builds.
Example for pinning versions:
- tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq && \ + tar=4.* gzip=1.* dbus-1-devel=1.* lsb-release=* graphviz-devel=* libnsl-devel=* libcurl-devel=* libjson-c-devel=* libacl-devel=* jq=1.* && \
30-33: Document the purpose of override variables.The purpose of
REPO_OVERRIDEandCOMMIT_ID_OVERRIDEvariables is not clear.Add comments to explain the purpose of these variables:
# Build nfs-ganesha +# REPO_OVERRIDE: Optional. Override the default repository URL for nfs-ganesha +# COMMIT_ID_OVERRIDE: Optional. Override the default commit ID for nfs-ganesha RUN export REPO_OVERRIDE="" && \ export COMMIT_ID_OVERRIDE="" && \ bash /usr/src/dep-versions/scripts/build-nfs-ganesha.sh "${REPO_OVERRIDE}" "${COMMIT_ID_OVERRIDE}"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 50-50: scripts/package#L50
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 22-22: scripts/package#L22
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 52-52: package/Dockerfile#L52
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 52-52: package/Dockerfile#L52
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (2)
scripts/package (1)
64-71: LGTM!The buildx command correctly passes the new build arguments to the Dockerfile.
package/Dockerfile (1)
5-6: LGTM!The build arguments are properly defined and align with the changes in scripts/package.
…rn/dep-versions Longhorn 10208 Signed-off-by: Derek Su <[email protected]>
Longhorn 10208 Signed-off-by: Derek Su <[email protected]>
derekbit
force-pushed
the
issue-10208
branch
from
e8cf324 to
3a1506e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/package (2)
16-22: 🛠️ Refactor suggestionImprove architecture detection and command substitution.
The architecture detection can be more robust:
- Add support for all possible architectures
- Use modern command substitution syntax
Apply this diff to improve the code:
-arch=$(uname -m) -if [ "$arch" == "aarch64" ]; then - ARCH="arm64" -else - ARCH="amd64" -fi -APIVERSION=`./bin/longhorn-share-manager-"$ARCH" version --client-only|jq ".clientVersion.apiVersion"` +# Detect system architecture +arch=$(uname -m) +case "$arch" in + "x86_64") + ARCH="amd64" + ;; + "aarch64") + ARCH="arm64" + ;; + *) + echo "Error: Unsupported architecture: $arch" + exit 1 + ;; +esac + +# Get API version using architecture-specific binary +APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# Detect system architecture arch=$(uname -m) case "$arch" in "x86_64") ARCH="amd64" ;; "aarch64") ARCH="arm64" ;; *) echo "Error: Unsupported architecture: $arch" exit 1 ;; esac # Get API version using architecture-specific binary APIVERSION=$(./bin/longhorn-share-manager-"$ARCH" version --client-only | jq -r ".clientVersion.apiVersion")🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 22-22: scripts/package#L22
Use $(...) notation instead of legacy backticks.... (SC2006)
37-54: 🛠️ Refactor suggestionSimplify branch detection and improve error handling.
Based on past review comments,
versions.jsonis not needed and is only used to check if the branch exists.Apply this diff to improve the code:
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 + # Check if branch exists, fallback to main + if wget -q --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/"; then echo "${branch}" else + echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.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") # Check if branch exists, fallback to main if wget -q --spider "https://raw.githubusercontent.com/longhorn/dep-versions/${branch}/"; then echo "${branch}" else echo "Warning: Branch ${branch} not found, falling back to main" echo "main" fi }🧰 Tools
🪛 GitHub Check: CodeFactor
[notice] 49-49: scripts/package#L49
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
🧹 Nitpick comments (2)
scripts/package (1)
56-57: Add error handling for git tag retrieval.The git tag command should handle the case when no tags are found.
Apply this diff to improve error handling:
SRC_BRANCH=$(get_branch) -SRC_TAG=$(git tag --points-at HEAD | head -n 1) +SRC_TAG=$(git tag --points-at HEAD | head -n 1 || echo "") +if [ -z "$SRC_TAG" ]; then + echo "Warning: No tags found pointing at HEAD" +fipackage/Dockerfile (1)
18-21: Consider pinning package versions for reproducible builds.While the package list is comprehensive, consider pinning package versions to ensure reproducible builds.
Example:
- tar gzip dbus-1-devel lsb-release graphviz-devel libnsl-devel libcurl-devel libjson-c-devel libacl-devel jq + tar=2.* gzip=1.* dbus-1-devel=1.* lsb-release=* graphviz-devel=* libnsl-devel=* libcurl-devel=* libjson-c-devel=* libacl-devel=* jq=*
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(3 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
scripts/package
[notice] 49-49: scripts/package#L49
Check exit code directly with e.g. 'if mycmd;', not indirectly with $?. (SC2181)
[notice] 22-22: scripts/package#L22
Use $(...) notation instead of legacy backticks .... (SC2006)
package/Dockerfile
[warning] 52-52: package/Dockerfile#L52
Specify version with zypper install -y <package>=<version>. (DL3037)
[warning] 52-52: package/Dockerfile#L52
zypper clean missing after zypper use. (DL3036)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build binaries
- GitHub Check: Summary
🔇 Additional comments (4)
scripts/package (2)
28-35: LGTM! Well-structured version conversion function.The function correctly extracts major and minor version components using regex and handles invalid versions appropriately.
63-70: LGTM! Well-structured build command.The build command correctly uses the new arguments for architecture, branch, and tag.
package/Dockerfile (2)
5-6: LGTM! Well-defined build arguments.The build arguments correctly match those used in scripts/package.
30-33: LGTM! Proper script execution setup.The build script execution is well-structured with proper environment variable setup.
|
@mergify backport v1.8.x |
✅ Backports have been created
|
longhorn/longhorn#10208
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