feat: download nodejs and install on legacy OS versions #970
Conversation
WalkthroughThe pull request introduces modifications to the XML configuration of the Unraid plugin located in Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
16-18: Consider architecture compatibility.The Node.js binary is specifically for x64 architecture (
linux-x64.tar.xz), but there are no checks to ensure compatibility with the system architecture. This could lead to installation failures on non-x64 systems.Consider:
- Adding architecture validation in the installation script
- Providing binaries for other architectures if needed
- Adding clear documentation about architecture requirements
Also applies to: 120-125
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(2 hunks)
🔇 Additional comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
116-116: LGTM: Version constraint addition.
The minimum version constraint addition is appropriate and helps prevent installation on unsupported versions.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
18-18: Consider using SHA256 in download URL for version pinning.While SHA256 verification is implemented, consider using the SHA256 in the download URL for additional security:
https://nodejs.org/dist/v20.18.1/node-v20.18.1-linux-x64.tar.xz?sha256=c6fa75c841cbffac851678a472f2a5bd612fff8308ef39236190e1f8dbb0e567
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(3 hunks)
🔇 Additional comments (3)
plugin/plugins/dynamix.unraid.net.plg (3)
16-18: Rename entities to match the Node.js version.
The entity names (NODE_612_*) suggest Node.js v6.12 but contain values for v20.18.1. This naming inconsistency could lead to confusion.
336-358: LGTM: Comprehensive file restoration list.
The file restoration list is well-structured and includes all necessary system and plugin files.
130-130:
Fix installation command syntax and add cleanup.
The current installation command has several issues:
- Syntax error in the tar extraction (missing target directory)
- No cleanup of extracted files
- Target directory might not exist
Apply this diff to fix the issues:
- tar -xvf /boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ} -C /usr/local/bin/;
+ mkdir -p /usr/local/bin && \
+ tar -xvf /boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ} && \
+ mv node-v20.18.1-linux-x64/* /usr/local/bin/ && \
+ rm -rf node-v20.18.1-linux-x64Likely invalid or redundant comment.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
357-359: Consider adding error handling to file restoration.While the current implementation works, it could benefit from:
- Error handling for failed moves
- Logging of restoration status
Apply this diff to enhance the implementation:
- for FILE in "${FILES_TO_RESTORE[@]}"; do - [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" - done + for FILE in "${FILES_TO_RESTORE[@]}"; do + if [[ -f "$FILE-" ]]; then + if mv -f "$FILE-" "$FILE"; then + echo "Restored: $FILE" + else + echo "Failed to restore: $FILE" + fi + else + echo "Backup not found: $FILE-" + fi + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(3 hunks)
🔇 Additional comments (3)
plugin/plugins/dynamix.unraid.net.plg (3)
16-18: Rename entities to match the Node.js version.
The entity names (NODE_612_*) suggest Node.js v6.12 but contain values for v20.18.1. This naming inconsistency could lead to confusion.
113-118: LGTM! Well-structured file element with proper constraints.
The file element is correctly implemented with:
- Version constraints using the
maxattribute - SHA256 checksum for integrity verification
- Direct download URL from nodejs.org
333-355: LGTM! Comprehensive file restoration list.
The array is well-structured with all necessary files for restoration.
| <FILE Run="/bin/bash" Method="install"> | ||
| <INLINE> | ||
| NODE_UNRIAD_612_TXZ="&NODE_612_FILENAME;" | ||
| <![CDATA[ | ||
| if [[ -f "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" ]]; then | ||
| tar --strip-components=1 -xf /boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ} -C /usr/local/; | ||
| fi | ||
| exit 0; | ||
| ]]> | ||
| </INLINE> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup to the installation script.
The current implementation should be enhanced with:
- Error handling for the tar extraction
- Cleanup of temporary files
- Verification of successful installation
Apply this diff to improve the script:
- <![CDATA[
- if [[ -f "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" ]]; then
- tar --strip-components=1 -xf /boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ} -C /usr/local/;
- fi
-
- exit 0;
- ]]>
+ <![CDATA[
+ if [[ -f "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" ]]; then
+ echo "Installing Node.js..."
+ TEMP_DIR=$(mktemp -d)
+ if tar -xf "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" -C "${TEMP_DIR}"; then
+ if cp -rf "${TEMP_DIR}"/*/bin/* /usr/local/bin/ && \
+ cp -rf "${TEMP_DIR}"/*/lib/* /usr/local/lib/ && \
+ cp -rf "${TEMP_DIR}"/*/include/* /usr/local/include/; then
+ echo "Node.js installation successful"
+ else
+ echo "Failed to copy Node.js files"
+ exit 1
+ fi
+ else
+ echo "Failed to extract Node.js archive"
+ exit 1
+ fi
+ rm -rf "${TEMP_DIR}"
+ else
+ echo "Node.js archive not found"
+ exit 1
+ fi
+
+ # Verify installation
+ if ! command -v node >/dev/null 2>&1; then
+ echo "Node.js installation verification failed"
+ exit 1
+ fi
+
+ exit 0
+ ]]>📝 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.
| <FILE Run="/bin/bash" Method="install"> | |
| <INLINE> | |
| NODE_UNRIAD_612_TXZ="&NODE_612_FILENAME;" | |
| <![CDATA[ | |
| if [[ -f "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" ]]; then | |
| tar --strip-components=1 -xf /boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ} -C /usr/local/; | |
| fi | |
| exit 0; | |
| ]]> | |
| </INLINE> | |
| <FILE Run="/bin/bash" Method="install"> | |
| <INLINE> | |
| NODE_UNRIAD_612_TXZ="&NODE_612_FILENAME;" | |
| <![CDATA[ | |
| if [[ -f "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" ]]; then | |
| echo "Installing Node.js..." | |
| TEMP_DIR=$(mktemp -d) | |
| if tar -xf "/boot/config/plugins/dynamix.my.servers/${NODE_UNRIAD_612_TXZ}" -C "${TEMP_DIR}"; then | |
| if cp -rf "${TEMP_DIR}"/*/bin/* /usr/local/bin/ && \ | |
| cp -rf "${TEMP_DIR}"/*/lib/* /usr/local/lib/ && \ | |
| cp -rf "${TEMP_DIR}"/*/include/* /usr/local/include/; then | |
| echo "Node.js installation successful" | |
| else | |
| echo "Failed to copy Node.js files" | |
| exit 1 | |
| fi | |
| else | |
| echo "Failed to extract Node.js archive" | |
| exit 1 | |
| fi | |
| rm -rf "${TEMP_DIR}" | |
| else | |
| echo "Node.js archive not found" | |
| exit 1 | |
| fi | |
| # Verify installation | |
| if ! command -v node >/dev/null 2>&1; then | |
| echo "Node.js installation verification failed" | |
| exit 1 | |
| fi | |
| exit 0 | |
| ]]> | |
| </INLINE> |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
328-354: Improve file restoration mechanism robustness.The current implementation could benefit from better error handling and logging:
- Source file validation
- Move operation error handling
- Restoration action logging
Apply this diff to enhance the file restoration mechanism:
FILES_TO_RESTORE=( "/usr/local/emhttp/plugins/dynamix/DisplaySettings.page" "/usr/local/emhttp/plugins/dynamix/Registration.page" # ... other files ... ) + # Initialize counter for restored files + RESTORED_COUNT=0 + FAILED_COUNT=0 + for FILE in "${FILES_TO_RESTORE[@]}"; do - [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" + if [[ -f "$FILE-" ]]; then + if mv -f "$FILE-" "$FILE" 2>/dev/null; then + echo "Restored: $FILE" + ((RESTORED_COUNT++)) + else + echo "Failed to restore: $FILE" + ((FAILED_COUNT++)) + fi + else + echo "Backup not found: $FILE-" + ((FAILED_COUNT++)) + fi done + + # Report restoration summary + echo "File restoration complete: $RESTORED_COUNT restored, $FAILED_COUNT failed" + [[ $FAILED_COUNT -gt 0 ]] && echo "⚠️ Some files could not be restored"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(3 hunks)
🔇 Additional comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
13-15: LGTM! Node.js entities are well-defined.
The entities are properly structured with clear naming, include security checksum, and use the official Node.js distribution URL.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
rc.unraid-apiscript for better feedback.