chore: versions of dependent packages are centrally managed in longhorn/dep-versions #831
Conversation
WalkthroughThis pull request updates the dependency management process by modifying both the Dockerfile and the packaging script. The Dockerfile now accepts a build argument ( Changes
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
|
derekbit
requested review from
innobead,
PhanLe1010,
c3y1huang,
shuo-wu,
mantissahz,
james-munson,
DamiaSan and
COLDTURNIP
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/package (2)
61-72: Add input validation for version content.While the function handles missing file errors, it doesn't validate the content of the version file. Consider adding validation to ensure the version string is not empty and matches the expected format.
Apply this diff to add input validation:
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") + if [[ -z "$version" ]]; then + echo "Error: Version file is empty." + exit 1 + fi + local branch=$(convert_version_to_major_minor_x "$version") + if [[ "$branch" == "Invalid version format: $version" ]]; then + echo "Error: Invalid version format in version file." + exit 1 + fi echo "${branch}" }
76-82: Document the version file fallback strategy.The code implements a smart fallback strategy for fetching versions.json, but it would benefit from a comment explaining this behavior to future maintainers.
Add a comment before the wget commands:
+# First attempt to fetch versions.json from the branch matching the current version. +# If that fails, fall back to the main branch which should contain the latest versions. wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/${BRANCH}/versions.json" -O package/versions.json || { echo "Failed to fetch from branch $BRANCH, trying main branch..." wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/main/versions.json" -O package/versions.json || { echo "Failed to fetch from main branch. Exiting." exit 1 } }package/Dockerfile (1)
89-89: Optimize pip installation by disabling cache.To reduce the image size and follow best practices, disable pip cache when installing requirements.
Apply this diff:
- pip3 install -r ./scripts/pkgdep/requirements.txt && \ + pip3 install --no-cache-dir -r ./scripts/pkgdep/requirements.txt && \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(5 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
package/Dockerfile
[warning] 79-79: package/Dockerfile#L79
Avoid use of cache directory with pip. Use pip install --no-cache-dir <package>. (DL3042)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build ARM64 binaries
- GitHub Check: Build AMD64 binaries
- GitHub Check: Summary
🔇 Additional comments (4)
scripts/package (1)
85-89: LGTM!The Docker build command is well-structured and correctly passes the DEP_VERSION_FILE argument.
package/Dockerfile (3)
5-5: LGTM!The changes to the gobuilder stage correctly implement the centralized version management approach:
- Added DEP_VERSION_FILE argument
- Installed jq for JSON parsing
- Copies and uses versions.json for dependency information
Also applies to: 13-13, 19-19, 22-23
40-40: LGTM!The changes to the cbuilder stage consistently implement the centralized version management approach across all dependencies.
Also applies to: 54-55, 56-56, 59-60, 69-70, 79-80, 104-105, 117-118
128-192: LGTM!The release stage correctly uses the binaries built in previous stages and is not directly affected by the version management changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/package (1)
66-72: Consider enhancing error handling and file validation.While the fallback logic is good, consider these improvements:
- Remove
-qflag from wget to see error details- Validate JSON file structure after download
Apply this diff:
-wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/${BRANCH}/versions.json" -O package/versions.json || { +wget "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/${BRANCH}/versions.json" -O package/versions.json || { echo "Failed to fetch from branch $BRANCH, trying main branch..." - wget -q "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/main/versions.json" -O package/versions.json || { + wget "https://raw.githubusercontent.com/longhorn/dep-versions/refs/heads/main/versions.json" -O package/versions.json || { echo "Failed to fetch from main branch. Exiting." exit 1 } } + +# Validate JSON file +if ! jq empty package/versions.json 2>/dev/null; then + echo "Error: Invalid JSON file" + exit 1 +fipackage/Dockerfile (1)
89-89: Add --no-cache-dir to pip install.Using pip's cache directory in container builds is unnecessary and increases image size.
Apply this diff:
- pip3 install -r ./scripts/pkgdep/requirements.txt && \ + pip3 install --no-cache-dir -r ./scripts/pkgdep/requirements.txt && \
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(5 hunks)scripts/package(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeFactor
package/Dockerfile
[warning] 79-79: package/Dockerfile#L79
Avoid use of cache directory with pip. Use pip install --no-cache-dir <package>. (DL3042)
⏰ 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 (3)
scripts/package (1)
51-62: LGTM! Well-structured error handling.The function includes proper error handling for missing version file and clearly communicates errors.
package/Dockerfile (2)
5-5: LGTM! Clean implementation of centralized version management.The changes effectively integrate the new version management system in the Go build stage.
Also applies to: 13-13, 19-19, 22-29
40-40: LGTM! Consistent implementation across dependencies.The changes maintain consistency in version management across all dependencies.
Also applies to: 54-56, 59-122
There was a problem hiding this comment.
Have we considered building a versioned lib image when updating the dep version file (that should be release version based) and the image includes all necessary dependent libraries (source code or prebuilt), so it can be just used here and it's not required to repeat the process of pulling and building code?
Yes, we can design the flow. One concern is the versioned lib image might make the development more time-consuming. |
…rn/dep-versions Longhorn 10208 Signed-off-by: Derek Su <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
package/Dockerfile (1)
89-90: Optimize pip Installation Command
The commandpip3 install -r ./scripts/pkgdep/requirements.txtshould consider disabling caching to avoid potential issues, as noted by static analysis (DL3042). A suggested update is:- pip3 install -r ./scripts/pkgdep/requirements.txt && \ + pip3 install --no-cache-dir -r ./scripts/pkgdep/requirements.txt && \
🧹 Nitpick comments (1)
package/Dockerfile (1)
22-29: Dynamic Checkout for go-spdk-helper via Dependency File
ExtractingGO_SPDK_HELPER_REPOandGO_SPDK_HELPER_COMMIT_IDfrom/versions.jsonusingjqis a robust move toward dynamic dependency management. Consider adding error handling if the expected keys are absent or if JSON parsing fails, to provide clearer build-time diagnostics.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Dockerfile(5 hunks)scripts/package(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/package
🧰 Additional context used
🪛 GitHub Check: CodeFactor
package/Dockerfile
[warning] 79-79: package/Dockerfile#L79
Avoid use of cache directory with pip. Use pip install --no-cache-dir <package>. (DL3042)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build ARM64 binaries
- GitHub Check: Build AMD64 binaries
- GitHub Check: Summary
🔇 Additional comments (11)
package/Dockerfile (11)
5-6: Centralized Version File Argument Added
The addition ofARG DEP_VERSION_FILE=versions.jsonensures that the Docker build can flexibly use a custom dependency version file, which is key to the centralized management of dependency versions.
19-20: Copying Dependency Version File into Image
UsingCOPY ${DEP_VERSION_FILE} /versions.jsonmakes the dependency version file available at build time and runtime. Ensure that the file is always present in the build context.
40-41: Consistent Dependency File Argument in Second Stage
Re-declaringARG DEP_VERSION_FILE=versions.jsonin the second build stage maintains consistency across stages. This is essential since build stages are isolated from one another.
54-55: Ensuring Availability of jq for JSON Parsing
Includingjqin the installation command (along with other necessary packages) guarantees that subsequent JSON parsing commands will execute correctly.
56-57: Copying Dependency File in Second Stage
TheCOPY ${DEP_VERSION_FILE} /versions.jsonstep in this stage mirrors the first stage and confirms that the dependency version file is available where needed.
59-65: Dynamic Retrieval of liblonghorn Version Info
ExtractingLIBLONGHORN_REPOandLIBLONGHORN_COMMIT_IDfrom the version file removes hardcoded commit IDs and improves maintainability. Ensure that the JSON file consistently uses the expected key names.
69-76: Dynamic Version Extraction for TGT Build
As with other components, usingjqto fetchTGT_REPOandTGT_COMMIT_IDcentralizes dependency details. It may be beneficial to validate the JSON file structure beforehand to avoid silent failures.
79-88: Robust SPDK Build Process with Dynamic Version Extraction
The SPDK build section effectively usesjqto extract repository details and includes necessary adjustments withsed. This flexible approach is well-aligned with the new dependency management strategy.🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 79-79: package/Dockerfile#L79
Avoid use of cache directory with pip. Usepip install --no-cache-dir <package>. (DL3042)
104-114: Dynamic Checkout for libjson-c-devel
Usingjqto extractLIBJSONC_REPOandLIBJSONC_COMMIT_IDis consistent with the overall strategy. The multi-step build using CMake is clear and easy to maintain.
117-125: Dynamic Retrieval and Build of nvme-cli
The extraction of repository and commit information fornvme-cliviajqcontinues the pattern of centralized dependency management. Monitor that the JSON file remains accurate to avoid build failures.
1-193: Overall Centralization of Dependency Versions
The Dockerfile modifications across all build stages effectively centralize dependency version management by leveraging a JSON file (versions.json). This reduces duplication and improves maintainability. Ensure that the JSON file is validated and regularly updated to reflect the required versions for all dependent components.🧰 Tools
🪛 GitHub Check: CodeFactor
[warning] 79-79: package/Dockerfile#L79
Avoid use of cache directory with pip. Usepip install --no-cache-dir <package>. (DL3042)
My original plan is to build an RPM package for each library and install them into a BCI-micro image while building the Longhorn component image as AppCo. However, there are some challenges:
|
I thought each should be independent, because the libs defined in https://github.com/longhorn/dep-versions/blob/main/versions.json are all shared library based? Which parts are missing? Btw, I am also fine with this implementation. |
Got it. What you mentioned is like #831 (comment). |
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