feat: track node version in slackware #949
Conversation
WalkthroughThe changes involve updates to the Unraid plugin's XML configuration, enhancing the installation and removal scripts for better clarity and functionality. Key modifications include the introduction of new entity definitions for Node.js and nghttp3, streamlined error handling, and improved validation processes. Additionally, the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)plugin/plugins/dynamix.unraid.net.plg (7)
The entity declarations are properly organized and include new entities for Node.js and nghttp3 components. Empty values are expected as they are populated during the build process.
The plugin metadata includes appropriate version requirements and basic changelog information.
The DNS check has a potential issue where the
The download configurations for Node.js and nghttp3 packages are properly set up with appropriate version constraints and SHA256 validation.
Multiple variable assignments are combined into one line. This reduces readability and maintainability.
The file manipulation operations include appropriate error handling and backup procedures. Also applies to: 519-522, 652-655 Line range hint The removal script includes appropriate version sourcing and user feedback. 🪧 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 (
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (4)
plugin/source/dynamix.unraid.net/pkg_build.sh (1)
65-86: Refactor redundant code segments into reusable functionsThe code for fetching, downloading, and processing the latest versions of Node.js and nghttp3 is almost identical. Refactoring these steps into a function can reduce code duplication and improve maintainability.
Consider creating a function like this:
fetch_latest_package() { local base_url="$1" local pattern="$2" local package_name local download_url local sha256sum_value package_name=$(wget -q -O- "${base_url}" | grep -o "${pattern}" | sort -V | tail -n 1) if [[ -z "${package_name}" ]]; then echo "Error: Failed to fetch the latest package matching pattern ${pattern}." exit 1 fi download_url="${base_url}${package_name}" wget -q "${download_url}" -O "/tmp/${package_name}" if [[ $? -ne 0 ]]; then echo "Error: Failed to download package ${package_name}." exit 1 fi sha256sum_value=$(sha256sum "/tmp/${package_name}" | cut -f 1 -d ' ') rm "/tmp/${package_name}" echo "${package_name};${download_url};${sha256sum_value}" }Then, replace the Node.js and nghttp3 fetching code with calls to this function:
# Get latest Node.js version node_info=$(fetch_latest_package "${base_node_url}" "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz") IFS=';' read -r latest_nodejs node_download_url node_sha256 <<< "${node_info}" # Get latest nghttp3 version nghttp3_info=$(fetch_latest_package "${base_nghttp3_url}" "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz") IFS=';' read -r latest_nghttp3 nghttp3_download_url nghttp3_sha256 <<< "${nghttp3_info}"This refactoring reduces repetition and centralizes the package fetching logic.
plugin/plugins/dynamix.unraid.net.plg (3)
26-29: Update the<CHANGES>section with relevant changelog entries.The
<CHANGES>section contains placeholder text and an initial release note. Consider updating this section with actual change logs to inform users about the updates and enhancements made in this version of the plugin.
Line range hint
228-288: Consolidate duplicate uninstall scripts to avoid redundancy.Lines 228-288 contain a PHP uninstall script that appears similar to the one in lines 161-221. Consider consolidating these scripts into a single function or script to avoid code duplication and reduce maintenance overhead.
Line range hint
358-804: Review complex installation script for potential errors and maintainability.The installation script between lines 358-804 is extensive and performs numerous operations, including file manipulation, patching, and conditional logic based on Unraid versions. Consider the following:
- Break down the script into smaller, modular functions for better readability.
- Add comments to complex sections for clarity.
- Ensure that all external commands and scripts are checked for success, and handle errors appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
plugin/plugins/dynamix.unraid.net.plg(19 hunks)plugin/source/dynamix.unraid.net/pkg_build.sh(2 hunks)
🔇 Additional comments (8)
plugin/source/dynamix.unraid.net/pkg_build.sh (1)
96-105: Ensure placeholders in the plugin file exist before substitution
The sed commands assume that the placeholders (NODE, NODE_SHA256, etc.) exist in the ${plgfile}. If they do not, the substitutions will have no effect.
Run the following script to confirm that the placeholders exist:
If any placeholders are missing, ensure they are added to the plugin file before running the script.
✅ Verification successful
The previous script failed because ${plgfile} was empty. Let's first locate the plugin file that contains these entity declarations, and then we can verify the placeholders within it.
All required placeholders are present in the plugin file
The verification confirms that all necessary XML entity placeholders (NODE, NODE_SHA256, NODE_TXZ, NGHTTP3, NGHTTP3_SHA256, NGHTTP3_TXZ) exist in ./plugin/plugins/dynamix.unraid.net.plg. The sed commands in the script will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all placeholders exist in the plugin file.
# Expected: Outputs of all grep commands should show the placeholders.
placeholders=("ENTITY NODE" "ENTITY NODE_SHA256" "ENTITY NODE_TXZ" "ENTITY NGHTTP3" "ENTITY NGHTTP3_SHA256" "ENTITY NGHTTP3_TXZ")
for placeholder in "${placeholders[@]}"; do
grep -q "${placeholder}" "${plgfile}" || echo "Placeholder ${placeholder} not found in ${plgfile}"
done
Length of output: 1352
Script:
#!/bin/bash
# First, let's find the plugin file
fd -t f -e plg --exec grep -l "ENTITY" {} \;
Length of output: 87
Script:
#!/bin/bash
# Now that we found the plugin file, let's check for the specific placeholders
echo "Checking placeholders in ./plugin/plugins/dynamix.unraid.net.plg:"
echo "-------------------------------------------"
grep "ENTITY NODE" ./plugin/plugins/dynamix.unraid.net.plg
grep "ENTITY NODE_SHA256" ./plugin/plugins/dynamix.unraid.net.plg
grep "ENTITY NODE_TXZ" ./plugin/plugins/dynamix.unraid.net.plg
grep "ENTITY NGHTTP3" ./plugin/plugins/dynamix.unraid.net.plg
grep "ENTITY NGHTTP3_SHA256" ./plugin/plugins/dynamix.unraid.net.plg
grep "ENTITY NGHTTP3_TXZ" ./plugin/plugins/dynamix.unraid.net.plg
Length of output: 992
plugin/plugins/dynamix.unraid.net.plg (7)
14-19: Verify that entity placeholders are properly populated during the build process.
Lines 14-19 define entities NODE, NODE_SHA256, NODE_TXZ, NGHTTP3, NGHTTP3_TXZ, and NGHTTP3_SHA256 with empty strings. Ensure that these entities will be correctly populated during the build or installation process to prevent issues with missing dependencies or incorrect URLs.
648-651: Ensure proper termination of PHP code when modifying files.
In lines 648-651, the script appends PHP functions to a file and adds the closing ?> tag. Make sure that the original file does not already contain a closing PHP tag to prevent syntax errors from duplicate ?> tags.
804-804: Confirm that the installation script exits successfully upon completion.
In line 804, after the installation steps, ensure that the script exits with a zero status to indicate success. This helps prevent unintended behavior during the plugin installation process.
109-120: Check compatibility with Unraid versions below 7.0.0-beta.5 when installing Node.js and nghttp3.
The installation of NODE and NGHTTP3 packages is confined to Unraid versions up to 7.0.0-beta.5 using the max="7.0.0-beta.5" attribute. Verify that this version constraint aligns with the intended compatibility and ensure that users on newer versions are not adversely affected.
Line range hint 161-221: Ensure proper error handling and security in PHP uninstall script.
The PHP uninstall script between lines 161-221 interacts with external services and performs file operations. Verify that:
- All external requests handle potential errors or exceptions.
- Sensitive data is handled securely, and no unintended data leakage occurs.
- File operations check for file existence and appropriate permissions before deletion.
687-687: Use proper syntax for the awk command to ensure reliable execution.
In line 687, an awk command is used to remove the [remote] section from a configuration file. Verify that the syntax and logic correctly handle all possible cases, including files with different formatting or unexpected content.
726-726:
Separate variable assignment from the cp command for proper syntax.
In line 726, the variable assignment FIND=... is combined with the cp command without separation. This can cause a syntax error. Place the variable assignment on a new line.
Apply this diff to fix the syntax error:
cp "$FILE" "$FILE-"
- FIND="location = \/robots.txt {"
+ FIND="location = \/robots.txt {"
# escape tabs and spaces
ADD="\ \ \ \ \ add_header Access-Control-Allow-Origin *; #robots.txt any origin"
sed -i "/${FIND}/a ${ADD}" "${FILE}"Likely invalid or redundant comment.
| if(output == "on"){print} | ||
| }' "${CFG}" >"${CFG}-new" && mv "${CFG}-new" "${CFG}" | ||
| CFG_CLEANED=1 | ||
| }' "${CFG}">"${CFG}-new" && mv "${CFG}-new" "${CFG}" CFG_CLEANED=1 |
There was a problem hiding this comment.
Separate variable assignment from the command for proper execution.
In line 687, the variable assignment CFG_CLEANED=1 is not properly separated from the preceding command. This can cause the variable assignment to fail or produce unexpected behavior. Place the variable assignment on a new line.
Apply this diff to fix the issue:
}' "${CFG}">"${CFG}-new" && mv "${CFG}-new" "${CFG}"
- CFG_CLEANED=1
+ CFG_CLEANED=1Alternatively, if the variable assignment is meant to be after the mv command:
}' "${CFG}">"${CFG}-new" && mv "${CFG}-new" "${CFG}"
+ CFG_CLEANED=1Committable suggestion skipped: line range outside the PR's diff.
| cp "$FILE" "$FILE-" | ||
| OLD="add_header X-Frame-Options 'SAMEORIGIN';" | ||
| NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" | ||
| cp "$FILE" "$FILE-" OLD="add_header X-Frame-Options 'SAMEORIGIN';" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" |
There was a problem hiding this comment.
Separate variable assignments from the cp command to prevent syntax errors.
In line 711, the cp command and the variable assignments OLD=... and NEW=... are on the same line without proper separation. This will cause a syntax error in Bash. Separate the commands onto separate lines.
Apply this diff to fix the syntax error:
if grep -q "SAMEORIGIN" "${FILE}"; then
CHANGED=yes
- cp "$FILE" "$FILE-" OLD="add_header X-Frame-Options 'SAMEORIGIN';" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";"
+ cp "$FILE" "$FILE-"
+ OLD="add_header X-Frame-Options 'SAMEORIGIN';"
+ NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";"
sed -i "s#${OLD}#${NEW}#" "${FILE}"
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.
| cp "$FILE" "$FILE-" OLD="add_header X-Frame-Options 'SAMEORIGIN';" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" | |
| if grep -q "SAMEORIGIN" "${FILE}"; then | |
| CHANGED=yes | |
| cp "$FILE" "$FILE-" | |
| OLD="add_header X-Frame-Options 'SAMEORIGIN';" | |
| NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" | |
| sed -i "s#${OLD}#${NEW}#" "${FILE}" | |
| fi |
| [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" | ||
| OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" | ||
| NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";" | ||
| [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";" |
There was a problem hiding this comment.
Separate variable assignments from the condition for correct execution.
In line 718, variable assignments OLD=... and NEW=... are included in the conditional statement without proper separation. This can cause syntax errors. Move the variable assignments to separate lines.
Apply this diff to fix the issue:
if ! grep -q "dev-my.myunraid.net:4000" "${FILE}"; then
CHANGED=yes
- [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";"
+ [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-"
+ OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";"
+ NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";"
sed -i "s#${OLD}#${NEW}#" "${FILE}"
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.
| [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";" | |
| if ! grep -q "dev-my.myunraid.net:4000" "${FILE}"; then | |
| CHANGED=yes | |
| [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" | |
| OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" | |
| NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";" | |
| sed -i "s#${OLD}#${NEW}#" "${FILE}" | |
| fi |
| FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" | ||
| FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" | ||
| DIR=/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components | ||
| FILE=/usr/local/emhttp/plugins/dynamix/DisplaySettings.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/DefaultPageLayout.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/ProvisionCert.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/Wrappers.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Downgrade.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/ShowChanges.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/MyServers.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers1.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers2.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/sbin/upgradepkg && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/data/server-state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" DIR=/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor long command sequence for readability and maintainability.
Line 315 combines multiple variable assignments and conditional commands into a single, excessively long line. This reduces readability and increases the risk of errors. Consider refactoring this line by using a loop or splitting the commands into multiple lines.
Apply this diff to refactor the code:
- FILE=/usr/local/emhttp/plugins/dynamix/DisplaySettings.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/DefaultPageLayout.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/ProvisionCert.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/Wrappers.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Downgrade.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/ShowChanges.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/MyServers.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers1.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers2.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/sbin/upgradepkg && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/data/server-state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" DIR=/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components
+ FILES=(
+ "/usr/local/emhttp/plugins/dynamix/DisplaySettings.page"
+ "/usr/local/emhttp/plugins/dynamix/Registration.page"
+ "/usr/local/emhttp/plugins/dynamix/include/DefaultPageLayout.php"
+ "/usr/local/emhttp/plugins/dynamix/include/ProvisionCert.php"
+ "/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php"
+ "/usr/local/emhttp/plugins/dynamix/include/Wrappers.php"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/Downgrade.page"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/include/ShowChanges.php"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck"
+ "/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/MyServers.page"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/Registration.page"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers1.php"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers2.php"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php"
+ "/sbin/upgradepkg"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/data/server-state.php"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php"
+ "/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php"
+ )
+
+ for FILE in "${FILES[@]}"; do
+ if [[ -f "$FILE-" ]]; then
+ mv -f "$FILE-" "$FILE"
+ fi
+ done
+
+ DIR="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components"📝 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=/usr/local/emhttp/plugins/dynamix/DisplaySettings.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/DefaultPageLayout.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/ProvisionCert.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix/include/Wrappers.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Downgrade.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/ShowChanges.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/MyServers.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/Registration.page && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers1.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers2.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/sbin/upgradepkg && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/data/server-state.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" FILE=/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php && [[ -f "$FILE-" ]] && mv -f "$FILE-" "$FILE" DIR=/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components | |
| FILES=( | |
| "/usr/local/emhttp/plugins/dynamix/DisplaySettings.page" | |
| "/usr/local/emhttp/plugins/dynamix/Registration.page" | |
| "/usr/local/emhttp/plugins/dynamix/include/DefaultPageLayout.php" | |
| "/usr/local/emhttp/plugins/dynamix/include/ProvisionCert.php" | |
| "/usr/local/emhttp/plugins/dynamix/include/UpdateDNS.php" | |
| "/usr/local/emhttp/plugins/dynamix/include/Wrappers.php" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/Downgrade.page" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/Update.page" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/include/ShowChanges.php" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/showchanges" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/scripts/unraidcheck" | |
| "/usr/local/emhttp/plugins/dynamix.plugin.manager/include/UnraidCheck.php" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/Connect.page" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/MyServers.page" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/Registration.page" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers1.php" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/include/myservers2.php" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/include/state.php" | |
| "/sbin/upgradepkg" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/data/server-state.php" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/include/reboot-details.php" | |
| "/usr/local/emhttp/plugins/dynamix.my.servers/include/translations.php" | |
| ) | |
| for FILE in "${FILES[@]}"; do | |
| if [[ -f "$FILE-" ]]; then | |
| mv -f "$FILE-" "$FILE" | |
| fi | |
| done | |
| DIR="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components" |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/pkg_build.sh(2 hunks)
🔇 Additional comments (1)
plugin/source/dynamix.unraid.net/pkg_build.sh (1)
Line range hint 65-108: Consider refactoring for better modularity and maintainability.
The script has similar patterns for handling Node.js and nghttp3 dependencies. Consider refactoring into functions to reduce code duplication and improve maintainability.
Here's a suggested approach:
# Function to fetch latest package version from Slackware
fetch_latest_version() {
local base_url="$1"
local package_pattern="$2"
local temp_file="/tmp/pkg_listing.$$"
if ! wget -q -O "${temp_file}" "${base_url}"; then
echo "Error: Failed to fetch package listing from ${base_url}"
rm -f "${temp_file}"
return 1
}
local latest_version
latest_version=$(grep -o "${package_pattern}" "${temp_file}" | sort -V | tail -n 1)
rm -f "${temp_file}"
if [[ -z "${latest_version}" ]]; then
echo "Error: Failed to determine latest version for pattern ${package_pattern}"
return 1
}
echo "${latest_version}"
}
# Function to download package and get checksum
download_and_checksum() {
local download_url="$1"
local package_name="$2"
local temp_file="/tmp/${package_name}"
if ! wget -q "${download_url}" -O "${temp_file}"; then
echo "Error: Failed to download ${package_name}"
rm -f "${temp_file}"
return 1
}
local checksum
checksum=$(sha256sum "${temp_file}" | cut -f 1 -d ' ')
rm -f "${temp_file}"
echo "${checksum}"
}
# Function to update plugin file entities
update_plugin_entities() {
local plgfile="$1"
local entity_prefix="$2"
local version="$3"
local sha256="$4"
local download_url="$5"
local backup_file="${plgfile}.bak"
# Create backup if it doesn't exist
[[ ! -f "${backup_file}" ]] && cp "${plgfile}" "${backup_file}"
# Update entities with verification
local entities=(
"${entity_prefix}|${version}"
"${entity_prefix}_SHA256|${sha256}"
"${entity_prefix}_TXZ|${download_url}"
)
for entity in "${entities[@]}"; do
local name="${entity%|*}"
local value="${entity#*|}"
sed -i.tmp -E "s#(ENTITY ${name}\s*)\".*\"#\1\"${value}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
if ! grep -q "ENTITY ${name}.*\"${value}\"" "${plgfile}"; then
echo "Error: Failed to update ${name} entity in plugin file"
mv "${backup_file}" "${plgfile}"
return 1
fi
done
return 0
}This refactoring:
- Reduces code duplication
- Improves error handling
- Makes the code more maintainable
- Makes it easier to add support for new packages
Let's verify if there are other packages that could benefit from this refactoring:
✅ Verification successful
Consider refactoring package handling for better maintainability
The verification shows that the package handling patterns are unique to plugin/source/dynamix.unraid.net/pkg_build.sh. While no other scripts in the codebase handle Slackware packages or similar download patterns, the proposed refactoring would still benefit this script by:
- Reducing code duplication between Node.js and nghttp3 handling
- Improving error handling and cleanup
- Making future package additions more maintainable
- Providing a consistent pattern for package management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar package handling patterns in other build scripts
rg -l 'wget.*mirrors\.slackware\.com.*txz' .
Length of output: 46
Script:
#!/bin/bash
# Let's try a broader search to find any Slackware package handling
rg -l 'slackware' .
# Also search for similar package download patterns
rg -l 'wget.*txz' .
# And check for other build scripts that might have similar patterns
fd 'build.*\.sh$'
Length of output: 306
Script:
#!/bin/bash
# Let's examine the content of the other pkg_build.sh to see if it has similar patterns
cat plugin/pkg_build.sh
# Also check if there are any other files that handle package downloads
rg -l 'download.*package|package.*download' .
Length of output: 192
| # Get latest node version (based on main_node_version) from slackware | ||
| main_node_version=$(find "${MAINDIR}/../.." -type f -path "*/api/.nvmrc" -exec sed 's/^v//' {} \;) | ||
| base_node_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/l/" | ||
| latest_nodejs=$(wget -q -O- "${base_node_url}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | ||
| if [[ -z "${latest_nodejs}" ]]; then | ||
| echo "Error: Failed to fetch the latest nodejs version." | ||
| exit 1 | ||
| fi | ||
| node_download_url="${base_node_url}${latest_nodejs}" | ||
| wget -q "${node_download_url}" -O "/tmp/${latest_nodejs}" | ||
| node_sha256=$(sha256sum "/tmp/${latest_nodejs}" | cut -f 1 -d ' ') | ||
|
|
||
| rm "/tmp/${latest_nodejs}" |
There was a problem hiding this comment.
Add error handling for base URL fetching and version validation.
The script needs additional error handling:
- The wget command for fetching the base URL directory listing needs error handling
- The main_node_version should be validated before use
Apply this diff to add the missing error handling:
main_node_version=$(find "${MAINDIR}/../.." -type f -path "*/api/.nvmrc" -exec sed 's/^v//' {} \;)
+if [[ -z "${main_node_version}" ]]; then
+ echo "Error: Failed to determine Node.js version from .nvmrc"
+ exit 1
+fi
base_node_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/l/"
-latest_nodejs=$(wget -q -O- "${base_node_url}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1)
+base_node_listing=$(wget -q -O- "${base_node_url}")
+if [[ $? -ne 0 ]]; then
+ echo "Error: Failed to fetch Node.js package listing from Slackware mirror."
+ exit 1
+fi
+latest_nodejs=$(echo "${base_node_listing}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1)📝 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.
| # Get latest node version (based on main_node_version) from slackware | |
| main_node_version=$(find "${MAINDIR}/../.." -type f -path "*/api/.nvmrc" -exec sed 's/^v//' {} \;) | |
| base_node_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/l/" | |
| latest_nodejs=$(wget -q -O- "${base_node_url}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | |
| if [[ -z "${latest_nodejs}" ]]; then | |
| echo "Error: Failed to fetch the latest nodejs version." | |
| exit 1 | |
| fi | |
| node_download_url="${base_node_url}${latest_nodejs}" | |
| wget -q "${node_download_url}" -O "/tmp/${latest_nodejs}" | |
| node_sha256=$(sha256sum "/tmp/${latest_nodejs}" | cut -f 1 -d ' ') | |
| rm "/tmp/${latest_nodejs}" | |
| # Get latest node version (based on main_node_version) from slackware | |
| main_node_version=$(find "${MAINDIR}/../.." -type f -path "*/api/.nvmrc" -exec sed 's/^v//' {} \;) | |
| if [[ -z "${main_node_version}" ]]; then | |
| echo "Error: Failed to determine Node.js version from .nvmrc" | |
| exit 1 | |
| fi | |
| base_node_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/l/" | |
| base_node_listing=$(wget -q -O- "${base_node_url}") | |
| if [[ $? -ne 0 ]]; then | |
| echo "Error: Failed to fetch Node.js package listing from Slackware mirror." | |
| exit 1 | |
| fi | |
| latest_nodejs=$(echo "${base_node_listing}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | |
| if [[ -z "${latest_nodejs}" ]]; then | |
| echo "Error: Failed to fetch the latest nodejs version." | |
| exit 1 | |
| fi | |
| node_download_url="${base_node_url}${latest_nodejs}" | |
| wget -q "${node_download_url}" -O "/tmp/${latest_nodejs}" | |
| node_sha256=$(sha256sum "/tmp/${latest_nodejs}" | cut -f 1 -d ' ') | |
| rm "/tmp/${latest_nodejs}" |
| # Get latest nghttp3 version | ||
| base_nghttp3_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/n/" | ||
| latest_nghttp3=$(wget -q -O- "${base_nghttp3_url}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | ||
| nghttp3_download_url="${base_nghttp3_url}${latest_nghttp3}" | ||
| wget -q "${nghttp3_download_url}" -O "/tmp/${latest_nghttp3}" | ||
| if [[ $? -ne 0 ]]; then | ||
| echo "Error: Failed to download nghttp3 package." | ||
| exit 1 | ||
| fi | ||
| nghttp3_sha256=$(sha256sum "/tmp/${latest_nghttp3}" | cut -f 1 -d ' ') | ||
| rm "/tmp/${latest_nghttp3}" |
There was a problem hiding this comment.
Add error handling for base URL fetching and cleanup.
The script needs additional error handling for the base URL fetch and should clean up temporary files in error paths.
Apply this diff to add the missing error handling and cleanup:
base_nghttp3_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/n/"
-latest_nghttp3=$(wget -q -O- "${base_nghttp3_url}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1)
+base_nghttp3_listing=$(wget -q -O- "${base_nghttp3_url}")
+if [[ $? -ne 0 ]]; then
+ echo "Error: Failed to fetch nghttp3 package listing from Slackware mirror."
+ exit 1
+fi
+latest_nghttp3=$(echo "${base_nghttp3_listing}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1)
+if [[ -z "${latest_nghttp3}" ]]; then
+ echo "Error: Failed to determine latest nghttp3 version."
+ exit 1
+fi
nghttp3_download_url="${base_nghttp3_url}${latest_nghttp3}"
wget -q "${nghttp3_download_url}" -O "/tmp/${latest_nghttp3}"
if [[ $? -ne 0 ]]; then
echo "Error: Failed to download nghttp3 package."
+ rm -f "/tmp/${latest_nghttp3}"
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.
| # Get latest nghttp3 version | |
| base_nghttp3_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/n/" | |
| latest_nghttp3=$(wget -q -O- "${base_nghttp3_url}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | |
| nghttp3_download_url="${base_nghttp3_url}${latest_nghttp3}" | |
| wget -q "${nghttp3_download_url}" -O "/tmp/${latest_nghttp3}" | |
| if [[ $? -ne 0 ]]; then | |
| echo "Error: Failed to download nghttp3 package." | |
| exit 1 | |
| fi | |
| nghttp3_sha256=$(sha256sum "/tmp/${latest_nghttp3}" | cut -f 1 -d ' ') | |
| rm "/tmp/${latest_nghttp3}" | |
| # Get latest nghttp3 version | |
| base_nghttp3_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/n/" | |
| base_nghttp3_listing=$(wget -q -O- "${base_nghttp3_url}") | |
| if [[ $? -ne 0 ]]; then | |
| echo "Error: Failed to fetch nghttp3 package listing from Slackware mirror." | |
| exit 1 | |
| fi | |
| latest_nghttp3=$(echo "${base_nghttp3_listing}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) | |
| if [[ -z "${latest_nghttp3}" ]]; then | |
| echo "Error: Failed to determine latest nghttp3 version." | |
| exit 1 | |
| fi | |
| nghttp3_download_url="${base_nghttp3_url}${latest_nghttp3}" | |
| wget -q "${nghttp3_download_url}" -O "/tmp/${latest_nghttp3}" | |
| if [[ $? -ne 0 ]]; then | |
| echo "Error: Failed to download nghttp3 package." | |
| rm -f "/tmp/${latest_nghttp3}" | |
| exit 1 | |
| fi | |
| nghttp3_sha256=$(sha256sum "/tmp/${latest_nghttp3}" | cut -f 1 -d ' ') | |
| rm "/tmp/${latest_nghttp3}" |
| # update node versions | ||
| sed -i -E "s#(ENTITY NODE\s*)\".*\"#\1\"${latest_nodejs}\"#g" "${plgfile}" | ||
| sed -i -E "s#(ENTITY NODE_SHA256\s*)\".*\"#\1\"${node_sha256}\"#g" "${plgfile}" | ||
| sed -i -E "s#(ENTITY NODE_TXZ\s*)\".*\"#\1\"${node_download_url}\"#g" "${plgfile}" | ||
|
|
||
| # update nghttp3 versions | ||
| sed -i -E "s#(ENTITY NGHTTP3\s*)\".*\"#\1\"${latest_nghttp3}\"#g" "${plgfile}" | ||
| sed -i -E "s#(ENTITY NGHTTP3_SHA256\s*)\".*\"#\1\"${nghttp3_sha256}\"#g" "${plgfile}" | ||
| sed -i -E "s#(ENTITY NGHTTP3_TXZ\s*)\".*\"#\1\"${nghttp3_download_url}\"#g" "${plgfile}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add verification for plugin file updates and backup.
The script should:
- Create a backup of the plugin file before modifications
- Verify that the sed replacements were successful
Apply this diff to add backup and verification:
+# Backup plugin file
+cp "${plgfile}" "${plgfile}.bak"
+
# update node versions
-sed -i -E "s#(ENTITY NODE\s*)\".*\"#\1\"${latest_nodejs}\"#g" "${plgfile}"
-sed -i -E "s#(ENTITY NODE_SHA256\s*)\".*\"#\1\"${node_sha256}\"#g" "${plgfile}"
-sed -i -E "s#(ENTITY NODE_TXZ\s*)\".*\"#\1\"${node_download_url}\"#g" "${plgfile}"
+sed -i.tmp -E "s#(ENTITY NODE\s*)\".*\"#\1\"${latest_nodejs}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NODE.*\"${latest_nodejs}\"" "${plgfile}"; then
+ echo "Error: Failed to update NODE entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
+sed -i.tmp -E "s#(ENTITY NODE_SHA256\s*)\".*\"#\1\"${node_sha256}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NODE_SHA256.*\"${node_sha256}\"" "${plgfile}"; then
+ echo "Error: Failed to update NODE_SHA256 entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
+sed -i.tmp -E "s#(ENTITY NODE_TXZ\s*)\".*\"#\1\"${node_download_url}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NODE_TXZ.*\"${node_download_url}\"" "${plgfile}"; then
+ echo "Error: Failed to update NODE_TXZ entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
# update nghttp3 versions
-sed -i -E "s#(ENTITY NGHTTP3\s*)\".*\"#\1\"${latest_nghttp3}\"#g" "${plgfile}"
-sed -i -E "s#(ENTITY NGHTTP3_SHA256\s*)\".*\"#\1\"${nghttp3_sha256}\"#g" "${plgfile}"
-sed -i -E "s#(ENTITY NGHTTP3_TXZ\s*)\".*\"#\1\"${nghttp3_download_url}\"#g" "${plgfile}"
+sed -i.tmp -E "s#(ENTITY NGHTTP3\s*)\".*\"#\1\"${latest_nghttp3}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NGHTTP3.*\"${latest_nghttp3}\"" "${plgfile}"; then
+ echo "Error: Failed to update NGHTTP3 entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
+sed -i.tmp -E "s#(ENTITY NGHTTP3_SHA256\s*)\".*\"#\1\"${nghttp3_sha256}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NGHTTP3_SHA256.*\"${nghttp3_sha256}\"" "${plgfile}"; then
+ echo "Error: Failed to update NGHTTP3_SHA256 entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
+sed -i.tmp -E "s#(ENTITY NGHTTP3_TXZ\s*)\".*\"#\1\"${nghttp3_download_url}\"#g" "${plgfile}" && rm "${plgfile}.tmp"
+if ! grep -q "ENTITY NGHTTP3_TXZ.*\"${nghttp3_download_url}\"" "${plgfile}"; then
+ echo "Error: Failed to update NGHTTP3_TXZ entity in plugin file"
+ mv "${plgfile}.bak" "${plgfile}"
+ exit 1
+fi
+
+# Remove backup if all updates were successful
+rm "${plgfile}.bak"📝 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.
| # update node versions | |
| sed -i -E "s#(ENTITY NODE\s*)\".*\"#\1\"${latest_nodejs}\"#g" "${plgfile}" | |
| sed -i -E "s#(ENTITY NODE_SHA256\s*)\".*\"#\1\"${node_sha256}\"#g" "${plgfile}" | |
| sed -i -E "s#(ENTITY NODE_TXZ\s*)\".*\"#\1\"${node_download_url}\"#g" "${plgfile}" | |
| # update nghttp3 versions | |
| sed -i -E "s#(ENTITY NGHTTP3\s*)\".*\"#\1\"${latest_nghttp3}\"#g" "${plgfile}" | |
| sed -i -E "s#(ENTITY NGHTTP3_SHA256\s*)\".*\"#\1\"${nghttp3_sha256}\"#g" "${plgfile}" | |
| sed -i -E "s#(ENTITY NGHTTP3_TXZ\s*)\".*\"#\1\"${nghttp3_download_url}\"#g" "${plgfile}" | |
| # Backup plugin file | |
| cp "${plgfile}" "${plgfile}.bak" | |
| # update node versions | |
| sed -i.tmp -E "s#(ENTITY NODE\s*)\".*\"#\1\"${latest_nodejs}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NODE.*\"${latest_nodejs}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NODE entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| sed -i.tmp -E "s#(ENTITY NODE_SHA256\s*)\".*\"#\1\"${node_sha256}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NODE_SHA256.*\"${node_sha256}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NODE_SHA256 entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| sed -i.tmp -E "s#(ENTITY NODE_TXZ\s*)\".*\"#\1\"${node_download_url}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NODE_TXZ.*\"${node_download_url}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NODE_TXZ entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| # update nghttp3 versions | |
| sed -i.tmp -E "s#(ENTITY NGHTTP3\s*)\".*\"#\1\"${latest_nghttp3}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NGHTTP3.*\"${latest_nghttp3}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NGHTTP3 entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| sed -i.tmp -E "s#(ENTITY NGHTTP3_SHA256\s*)\".*\"#\1\"${nghttp3_sha256}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NGHTTP3_SHA256.*\"${nghttp3_sha256}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NGHTTP3_SHA256 entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| sed -i.tmp -E "s#(ENTITY NGHTTP3_TXZ\s*)\".*\"#\1\"${nghttp3_download_url}\"#g" "${plgfile}" && rm "${plgfile}.tmp" | |
| if ! grep -q "ENTITY NGHTTP3_TXZ.*\"${nghttp3_download_url}\"" "${plgfile}"; then | |
| echo "Error: Failed to update NGHTTP3_TXZ entity in plugin file" | |
| mv "${plgfile}.bak" "${plgfile}" | |
| exit 1 | |
| fi | |
| # Remove backup if all updates were successful | |
| rm "${plgfile}.bak" |
|
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 (4)
.github/workflows/main.yml (2)
166-182: Enhance Node.js download script robustnessThe Node.js download script could benefit from additional error handling and validation:
- name: Download Node.js From Slackbuilds run: | # Get latest node version (based on main_node_version) from slackware main_node_version=$(find "${MAINDIR}/../.." -type f -path "*/api/.nvmrc" -exec sed 's/^v//' {} \;) + if [[ ! $main_node_version =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + echo "Error: Invalid version format in .nvmrc" + exit 1 + fi base_node_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/l/" + fallback_url="https://slackware.uk/slackware/slackware64-current/slackware64/l/" latest_nodejs=$(wget -q -O- "${base_node_url}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) if [[ -z "${latest_nodejs}" ]]; then - echo "Error: Failed to fetch the latest nodejs version." - exit 1 + echo "Warning: Failed to fetch from primary mirror, trying fallback..." + latest_nodejs=$(wget -q -O- "${fallback_url}" | grep -o "nodejs-${main_node_version}\.[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) + if [[ -z "${latest_nodejs}" ]]; then + echo "Error: Failed to fetch the latest nodejs version from all mirrors." + exit 1 + fi + base_node_url="${fallback_url}" fi node_download_url="${base_node_url}${latest_nodejs}" - if ! wget -q "${node_download_url}" -O "${{ github.workspace }}/plugin/archive/${latest_nodejs}"; then + if ! wget -q --timeout=30 "${node_download_url}" -O "${{ github.workspace }}/plugin/archive/${latest_nodejs}"; then echo "Error: Failed to download nodejs package." exit 1 fi node_sha256=$(sha256sum "${{ github.workspace }}/plugin/archive/${latest_nodejs}" | cut -f 1 -d ' ')🧰 Tools
🪛 actionlint
167-167: shellcheck reported issue in this script: SC2034:warning:14:1: node_sha256 appears unused. Verify use (or export if used externally)
(shellcheck)
183-192: Apply similar robustness improvements to nghttp3 downloadThe nghttp3 download script would benefit from the same improvements as the Node.js script:
- name: Download nghttp3 run: | # Get latest nghttp3 version base_nghttp3_url="https://mirrors.slackware.com/slackware/slackware64-current/slackware64/n/" + fallback_url="https://slackware.uk/slackware/slackware64-current/slackware64/n/" latest_nghttp3=$(wget -q -O- "${base_nghttp3_url}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) + if [[ -z "${latest_nghttp3}" ]]; then + echo "Warning: Failed to fetch from primary mirror, trying fallback..." + latest_nghttp3=$(wget -q -O- "${fallback_url}" | grep -o "nghttp3-[0-9.]*-x86_64-[0-9]*\.txz" | sort -V | tail -n 1) + if [[ -z "${latest_nghttp3}" ]]; then + echo "Error: Failed to fetch the latest nghttp3 version from all mirrors." + exit 1 + fi + base_nghttp3_url="${fallback_url}" + fi nghttp3_download_url="${base_nghttp3_url}${latest_nghttp3}" - if ! wget -q "${nghttp3_download_url}" -O "${{ github.workspace }}/plugin/archive/${latest_nghttp3}"; then + if ! wget -q --timeout=30 "${nghttp3_download_url}" -O "${{ github.workspace }}/plugin/archive/${latest_nghttp3}"; then echo "Error: Failed to download nghttp3 package." exit 1 fi nghttp3_sha256=$(sha256sum "${{ github.workspace }}/plugin/archive/${latest_nghttp3}" | cut -f 1 -d ' ')Note: The static analysis warning about
nghttp3_sha256being unused can be ignored as it's used in the build step.🧰 Tools
🪛 actionlint
183-183: shellcheck reported issue in this script: SC2034:warning:9:1: nghttp3_sha256 appears unused. Verify use (or export if used externally)
(shellcheck)
plugin/plugins/dynamix.unraid.net.plg (2)
464-466: Consider using sed for atomic file modification.While the current approach using head/tail is functional, using sed with an atomic move operation would be more robust:
- head -$((LINENUM-1)) "$FILE"> "$FILE~" - echo '<unraid-i18n-host><unraid-header-os-version></unraid-header-os-version></unraid-i18n-host>' >> "$FILE~" - tail +$((LINENUM+1)) "$FILE">> "$FILE~" + sed -i.bak "${LINENUM}c\\<unraid-i18n-host><unraid-header-os-version></unraid-header-os-version></unraid-i18n-host>" "$FILE"
649-652: Optimize multiple file appends.Multiple echo commands can be combined into a single operation for better efficiency.
- [[ -n "${ADDTEXT1}" ]] && echo "${ADDTEXT1}">>"${TMP}" - [[ -n "${ADDTEXT2}" ]] && echo "${ADDTEXT2}">>"${TMP}" - [[ -n "${ADDTEXT3}" ]] && echo "${ADDTEXT3}">>"${TMP}" - echo "?>">>"${TMP}" + { + [[ -n "${ADDTEXT1}" ]] && echo "${ADDTEXT1}" + [[ -n "${ADDTEXT2}" ]] && echo "${ADDTEXT2}" + [[ -n "${ADDTEXT3}" ]] && echo "${ADDTEXT3}" + echo "?>" + } >> "${TMP}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/main.yml(4 hunks)plugin/plugins/dynamix.unraid.net.plg(18 hunks)plugin/source/dynamix.unraid.net/pkg_build.sh(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/source/dynamix.unraid.net/pkg_build.sh
🧰 Additional context used
🪛 actionlint
.github/workflows/main.yml
167-167: shellcheck reported issue in this script: SC2034:warning:14:1: node_sha256 appears unused. Verify use (or export if used externally)
(shellcheck)
183-183: shellcheck reported issue in this script: SC2034:warning:9:1: nghttp3_sha256 appears unused. Verify use (or export if used externally)
(shellcheck)
🔇 Additional comments (5)
.github/workflows/main.yml (2)
193-202: LGTM: Environment variables are properly configured
The build plugin environment setup correctly exports all necessary variables including API version, checksums, and package filenames.
255-255: Verify wildcard file copying patterns
The workflow now uses wildcards to copy all .txz files. While this makes the workflow more flexible, please verify that:
- Only intended .txz files are present in the archive directory
- No temporary or intermediate .txz files are accidentally included
Also applies to: 310-310, 364-364
✅ Verification successful
Let me try to gather more context about how these .txz files are generated and where they are expected to be created.
From the output, I can see that the workflow downloads specific .txz files (nodejs and nghttp3) from URLs and then copies them. Let me check the complete workflow context around these downloads to understand the file handling better.
Wildcard file copying is safe and controlled
The workflow strictly controls which .txz files are present in the archive directory:
- Only two specific packages are downloaded: nodejs and nghttp3
- Downloads are version-controlled with explicit pattern matching
- Files are verified with SHA256 checksums
- The archive directory is not used for any other .txz files
The wildcard copying (cp archive/*.txz) is safe as the directory only contains these verified packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any unexpected .txz files in the archive directory
# Test: List all .txz files that would be copied
echo "Checking for .txz files that would be copied:"
fd -e txz . archive/
# Test: Verify expected patterns
echo -e "\nVerifying expected patterns:"
fd -e txz . archive/ | grep -E '(nodejs-[0-9.]+|nghttp3-[0-9.]+)-x86_64-[0-9]+\.txz' || echo "No matching Node.js or nghttp3 packages found"
Length of output: 576
Script:
#!/bin/bash
# Check the workflow file to understand the archive directory creation and file generation
echo "Checking workflow steps before the copy command:"
rg -B 10 "cp archive/\*.txz" .github/workflows/main.yml
echo -e "\nChecking all references to .txz files in the workflow:"
rg "\.txz" .github/workflows/main.yml
echo -e "\nChecking makepkg commands that might generate .txz files:"
rg "makepkg" .github/workflows/main.yml
Length of output: 1763
Script:
#!/bin/bash
# Check the complete context around .txz file downloads and handling
echo "Checking the complete download and archive handling context:"
rg -B 5 -A 5 "latest_nodejs|latest_nghttp3" .github/workflows/main.yml
echo -e "\nChecking archive directory creation and handling:"
rg -B 3 -A 3 "archive/|mkdir.*archive" .github/workflows/main.yml
Length of output: 4879
plugin/plugins/dynamix.unraid.net.plg (3)
14-19: LGTM: Improved entity declarations for dynamic version management.
The change to empty entity declarations allows for dynamic population of Node.js and nghttp3 versions during the build process, making version management more maintainable.
773-773: Separate multiple variable assignments for clarity.
The line combines multiple variable assignments without proper separation, which reduces readability and could lead to syntax errors.
Line range hint 808-821: LGTM: Clean and minimal removal script.
The removal script follows good practices with proper version handling and user feedback.
|
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 (5)
plugin/source/dynamix.unraid.net/pkg_build.sh (2)
68-71: Improve documentation for hardcoded Node.js version.The comment "Hardcoded to deal with the new -2 release breaking legacy Unraid versions" could be more descriptive. Consider:
- Document which legacy versions are affected
- Explain why version 20.18.0 specifically was chosen
- Add a TODO comment about when this can be removed
Add more detailed documentation:
-# Hardcoded to deal with the new -2 release breaking legacy Unraid versions +# TODO: Remove hardcoding when legacy support is no longer needed +# Hardcoded to Node.js 20.18.0 because: +# 1. Legacy Unraid versions (specify versions) break with newer -2 releases +# 2. This version provides the best compatibility while maintaining security
96-103: Consider implementing progressive validation.Currently, all entity updates are performed before validation begins. This means that if validation fails after several successful updates, we lose track of which entities were successfully updated. Consider validating each entity immediately after its update.
Here's a suggested approach:
# Function to update and validate entity +update_and_validate_entity() { + local entity="$1" + local value="$2" + + # Update entity + sed -i.tmp -E "s#(ENTITY ${entity}\\s*)\".*\"#\\1\"${value}\"#g" "${plgfile}" && rm "${plgfile}.tmp" + + # Validate update + if ! grep -q "ENTITY ${entity} \"${value}\"" "${plgfile}"; then + echo "Error: Failed to update ENTITY ${entity} in ${plgfile}" + return 1 + fi + return 0 +} -# update node versions -sed -i -E "s#(ENTITY NODEJS_FILENAME\s*)\".*\"#\1\"${NODEJS_FILENAME}\"#g" "${plgfile}" -sed -i -E "s#(ENTITY NODEJS_SHA256\s*)\".*\"#\1\"${NODEJS_SHA256}\"#g" "${plgfile}" -sed -i -E "s#(ENTITY NODEJS_TXZ\s*)\".*\"#\1\"${NODEJS_TXZ}\"#g" "${plgfile}" +# Update and validate node versions +update_and_validate_entity "NODEJS_FILENAME" "${NODEJS_FILENAME}" || exit 1 +update_and_validate_entity "NODEJS_SHA256" "${NODEJS_SHA256}" || exit 1 +update_and_validate_entity "NODEJS_TXZ" "${NODEJS_TXZ}" || exit 1This approach:
- Validates each update immediately
- Provides better error messages
- Allows for easier debugging of failures
.github/workflows/main.yml (1)
208-215: Improve readability of the if conditionThe current implementation has trailing spaces and could be more readable.
Apply this diff to improve readability:
- if [ -z "${API_VERSION}" ] || - [ -z "${API_MD5}" ] || - [ -z "${API_SHA256}" ] || - [ -z "${NGHTTP3_FILENAME}" ] || - [ -z "${NGHTTP3_SHA256}" ]; then + if [ -z "${API_VERSION}" ] \ + || [ -z "${API_MD5}" ] \ + || [ -z "${API_SHA256}" ] \ + || [ -z "${NGHTTP3_FILENAME}" ] \ + || [ -z "${NGHTTP3_SHA256}" ]; then🧰 Tools
🪛 yamllint
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 209-209: trailing spaces
(trailing-spaces)
[error] 210-210: trailing spaces
(trailing-spaces)
[error] 211-211: trailing spaces
(trailing-spaces)
plugin/plugins/dynamix.unraid.net.plg (2)
776-778: Improve variable assignments for better readability.Multiple variable assignments are combined into one line without proper separators, which reduces readability.
Apply this diff to improve the code:
- plgWebComponentPath="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components" - backupWebComponentPath="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components-" - plgManifestTs=$(extract_ts "$plgWebComponentPath/manifest.json") + plgWebComponentPath="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components" + backupWebComponentPath="/usr/local/emhttp/plugins/dynamix.my.servers/unraid-components-" + plgManifestTs=$(extract_ts "$plgWebComponentPath/manifest.json")
715-715: Improve variable assignments in nginx configuration section.Variable assignments are combined with commands without proper separation.
Apply this diff to improve the code:
- cp "$FILE" "$FILE-" OLD="add_header X-Frame-Options 'SAMEORIGIN';" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" + cp "$FILE" "$FILE-" + OLD="add_header X-Frame-Options 'SAMEORIGIN';" + NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" - [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";" + [[ ! -f "$FILE-" ]] && cp "$FILE" "$FILE-" + OLD="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect.myunraid.net/\";" + NEW="add_header Content-Security-Policy \"frame-ancestors 'self' https://connect-staging.myunraid.net https://connect.myunraid.net/ https://dev-my.myunraid.net:4000/\";"Also applies to: 722-722
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/main.yml(4 hunks)plugin/plugins/dynamix.unraid.net.plg(18 hunks)plugin/source/dynamix.unraid.net/pkg_build.sh(2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/main.yml
169-169: shellcheck reported issue in this script: SC2086:info:15:44: Double quote to prevent globbing and word splitting
(shellcheck)
169-169: shellcheck reported issue in this script: SC2086:info:16:40: Double quote to prevent globbing and word splitting
(shellcheck)
188-188: shellcheck reported issue in this script: SC2086:info:10:46: Double quote to prevent globbing and word splitting
(shellcheck)
188-188: shellcheck reported issue in this script: SC2086:info:11:44: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/main.yml
[error] 208-208: trailing spaces
(trailing-spaces)
[error] 209-209: trailing spaces
(trailing-spaces)
[error] 210-210: trailing spaces
(trailing-spaces)
[error] 211-211: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
.github/workflows/main.yml (2)
186-199: Previous review comment about nghttp3 download improvements is still applicable
🧰 Tools
🪛 actionlint
188-188: shellcheck reported issue in this script: SC2086:info:10:46: Double quote to prevent globbing and word splitting
(shellcheck)
188-188: shellcheck reported issue in this script: SC2086:info:11:44: Double quote to prevent globbing and word splitting
(shellcheck)
269-269: LGTM! Good improvement in file handling flexibility
The changes to use wildcards for copying files make the workflow more maintainable and adaptable to future changes.
Also applies to: 322-324, 378-378
plugin/plugins/dynamix.unraid.net.plg (5)
3-20: LGTM: Entity definitions are well-structured.
The new entity definitions for Node.js and nghttp3 packages provide better modularity and version tracking capabilities.
23-29: LGTM: Plugin metadata is properly configured.
The plugin metadata includes all necessary attributes and appropriate version constraints.
Line range hint 31-108: LGTM: Installation validation scripts are robust and properly fixed.
The validation scripts now correctly handle:
- Prevention of concurrent production/staging installations
- DNS checks with proper variable assignments
- SHA256 validation with proper command separation
111-135: LGTM: Package installation sections are well-defined.
The Node.js and nghttp3 package installations include:
- Proper version constraints
- SHA256 validation for package integrity
- Clear file naming and organization
Line range hint 813-827: LGTM: Removal script is clean and straightforward.
The script properly handles:
- Version information sourcing
- User feedback
- Clean exit
| - name: Download Node.js From Slackbuilds (skipped due to node.js issues) | ||
| if: false |
There was a problem hiding this comment.
💡 Codebase verification
Based on the git history, I can now generate the final response:
Improve documentation for skipped Node.js download step
The Node.js download step was recently disabled without proper documentation. The git history shows this was a working step that was explicitly disabled with if: false. To maintain code quality:
- Add a detailed comment explaining the specific Node.js issues encountered
- Include a reference to any related issue/PR that tracks this problem
- Consider adding a TODO with an estimated timeline for re-enabling this step
- If this is a permanent change, consider removing the step entirely instead of keeping it disabled
🔗 Analysis chain
Document Node.js download issues and resolution plan
The step is currently skipped with a vague comment. Please:
- Document the specific Node.js issues that led to skipping this step
- Create a tracking issue for resolution
- Add a link to the tracking issue in the comment
Let me check the workflow file to understand the context better.
Let me check the .nvmrc file to understand the Node.js version requirement.
Let me check if there are any recent changes or commits related to this Node.js download step.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing issues about Node.js download problems
gh issue list --search "in:title node.js download" --json number,title,url
Length of output: 79
Script:
#!/bin/bash
# Check the workflow file for any comments or related steps around Node.js
rg -A 5 -B 5 "node\.js" .github/workflows/main.yml -i
Length of output: 576
Script:
#!/bin/bash
# Check for .nvmrc file and its content
fd -t f .nvmrc -x cat {}
Length of output: 28
Script:
#!/bin/bash
# Check git history for changes related to Node.js download step
git log -p -- .github/workflows/main.yml | grep -B 5 -A 5 "Download Node.js"
Length of output: 2434
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
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 (2)
plugin/source/dynamix.unraid.net/pkg_build.sh (2)
68-71: Improve version management and documentation.While hardcoding the Node.js version helps with legacy compatibility, it could lead to maintenance challenges. Consider:
- Adding more detailed documentation about which legacy versions are affected.
- Moving this configuration to a separate file for easier updates.
Consider creating a
versions.configfile:-# Hardcoded to deal with the new -2 release breaking legacy Unraid versions -NODEJS_FILENAME="nodejs-20.18.0-x86_64-1.txz" -NODEJS_TXZ="https://stable.dl.unraid.net/unraid-api/dependencies/${NODEJS_FILENAME}" -NODEJS_SHA256="332f22a2a6722e9fad92b8d1eeaded228a6499b7335b2b54ee99c55b4fe49742" +# Source version configurations +source "${MAINDIR}/config/versions.config"Create
config/versions.config:# Node.js version configuration # Pinned to 20.18.0 for compatibility with Unraid versions <= 6.9.x # Issue reference: [Add link to issue/documentation] NODEJS_FILENAME="nodejs-20.18.0-x86_64-1.txz" NODEJS_TXZ="https://stable.dl.unraid.net/unraid-api/dependencies/${NODEJS_FILENAME}" NODEJS_SHA256="332f22a2a6722e9fad92b8d1eeaded228a6499b7335b2b54ee99c55b4fe49742"
96-116: Enhance error reporting in validation.The validation logic is robust, but error messages could be more informative for debugging.
Consider this enhancement:
for entity in "${required_entities[@]}"; do entity_value=$(grep -oP "ENTITY ${entity} \"\K[^\"]*" "${plgfile}" || echo "") if [[ -z "${entity_value}" ]]; then - echo "Error: ENTITY ${entity} was not replaced correctly in ${plgfile}" + echo "Error: ENTITY ${entity} was not found or has no value in ${plgfile}" validation_failed=true elif [[ "${entity_value}" =~ ^[[:space:]]*$ ]]; then - echo "Error: ENTITY ${entity} has an empty value in ${plgfile}" + echo "Error: ENTITY ${entity} contains only whitespace in ${plgfile}" + echo "Expected format: <!ENTITY ${entity} \"some-non-empty-value\">" validation_failed=true fi done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
plugin/source/dynamix.unraid.net/pkg_build.sh(2 hunks)
🔇 Additional comments (1)
plugin/source/dynamix.unraid.net/pkg_build.sh (1)
82-90: LGTM!
The entity update logic is correct. Previous review comments already cover the necessary improvements for backup and verification.
| PLUGIN_URL="https://stable.dl.unraid.net/unraid-api/\&name;.plg" | ||
| MAIN_TXZ="https://stable.dl.unraid.net/unraid-api/${plugin}-${version}.txz" | ||
| API_TGZ="https://stable.dl.unraid.net/unraid-api/unraid-api-${API_VERSION}.tgz" | ||
| NGHTTP3_TXZ="https://stable.dl.unraid.net/unraid-api/${NGHTTP3_FILENAME}" |
There was a problem hiding this comment.
Add validation for NGHTTP3_FILENAME environment variable.
The script uses NGHTTP3_FILENAME to construct URLs without validating its presence. This could lead to invalid URLs if the variable is not set.
Add this validation at the beginning of the script:
[[ -z "${env}" ]] && echo "usage: [s|p]" && exit 1
+# Validate required environment variables
+if [[ -z "${NGHTTP3_FILENAME}" ]]; then
+ echo "Error: NGHTTP3_FILENAME environment variable is not set"
+ exit 1
+fiAlso applies to: 60-60, 65-65
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
plugin/plugins/dynamix.unraid.net.plg (2)
26-29: Consider enhancing the changes section.The current changes section is minimal. Consider adding more detailed information about:
- The addition of Node.js version tracking
- Any breaking changes or important updates
- Migration instructions if necessary
Line range hint
169-225: Enhance error handling in PHP cleanup script.The PHP cleanup script could benefit from:
- Error handling for file operations
- Logging of cleanup actions
- Status reporting for each operation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
plugin/plugins/dynamix.unraid.net.plg(18 hunks)plugin/source/dynamix.unraid.net/sbin/upgradepkg(0 hunks)
💤 Files with no reviewable changes (1)
- plugin/source/dynamix.unraid.net/sbin/upgradepkg
🔇 Additional comments (3)
plugin/plugins/dynamix.unraid.net.plg (3)
3-20: LGTM! Improved package management structure.
The entity declarations have been enhanced to better track Node.js and nghttp3 packages with proper version management and security verification through SHA256 hashes.
Line range hint 812-826: LGTM! Clean removal script implementation.
The removal script is concise and properly handles cleanup operations.
111-123: Verify version constraint for package installations.
Both Node.js and nghttp3 installations have a version constraint of max="7.0.0-beta.5". Please confirm this is the intended maximum version.
✅ Verification successful
Let me gather more context about version constraints and package configurations in the repository.
Let me search for more specific package and version information in the plg file.
Based on the analysis of the plg file and package configurations, I can now generate the final response:
Version constraint max="7.0.0-beta.5" is correctly set for Node.js and nghttp3 packages
The version constraint is intentionally set to prevent installation of Node.js and nghttp3 packages beyond version 7.0.0-beta.5, which is a deliberate compatibility restriction. This is consistent with the package management approach shown in the installation scripts, where specific versions of dependencies are managed to ensure stability and compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any version-specific package configurations
rg -A 2 'max="7.0.0-beta.5"'
Length of output: 441
Script:
#!/bin/bash
# Check for other version constraints in plg files
rg -A 2 'max="[^"]*"' --type xml
# Look for version-related variables or constants
rg -A 2 'VERSION|version' --type xml
# Check for any documentation about version constraints
fd README.md -X cat
Length of output: 5889
Script:
#!/bin/bash
# Check the entire plg file content to understand package versioning context
cat plugin/plugins/dynamix.unraid.net.plg
# Search for any package version definitions
rg "NODEJS|NGHTTP3" plugin/plugins/dynamix.unraid.net.plg
Length of output: 34604
| appendTextIfMissing() { | ||
| FILE="$1" | ||
| TEXT="$2" | ||
| FILE="$1" TEXT="$2" | ||
| if test -f "${FILE}" && ! grep -q "${TEXT}" "${FILE}" &>/dev/null; then | ||
| echo "${TEXT}" >>"${FILE}" | ||
| echo "${TEXT}">>"${FILE}" | ||
| fi |
There was a problem hiding this comment.
Improve file path security.
The appendTextIfMissing function should validate file paths to prevent directory traversal attacks. Consider:
- Validating file paths against a whitelist
- Using realpath to resolve symbolic links
- Adding path sanitization
This reverts commit dc1c4fb.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores