feat: use plugin file for install and uninstall #995
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Unraid Connect plugin's installation and removal processes. The uninstall method has been streamlined to directly remove specific directories and files, replacing the previous uninstall script. The installation script has been restructured to include detailed checks for necessary files and directories, explicitly installing the Unraid API. Enhanced error handling and user messaging have been added, while outdated comments have been removed for clarity. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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: 0
🧹 Nitpick comments (3)
plugin/plugins/dynamix.unraid.net.plg (1)
857-857: Schedule flash backup carefully.Enqueuing flash backup via
at -M nowensures it launches in the background without blocking. In rare cases where theatdservice or system clock might be unavailable, consider adding fallback handling to avoid silent failures.plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)
9-9: Clarify the no-op behavior for install().All install logic has shifted to the plugin file. Keeping this function as a no-op may cause confusion for those who expect it to perform an installation. A clarifying comment or removal of the function entirely would avoid confusion.
12-12: Clarify the no-op behavior for uninstall().Likewise, making the uninstall function a no-op defers all removal logic to the
.plgscript. Ensure that users who rely on this script to uninstall do not mistakenly believe it still performs cleanup. A comment or removal would clarify that the.plgmanages the uninstall.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
plugin/plugins/dynamix.unraid.net.plg(2 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(1 hunks)
🔇 Additional comments (2)
plugin/plugins/dynamix.unraid.net.plg (2)
327-329: Ensure no leftover logs or directories are needed before removal.
Removing all API-related folders and files is consistent with a complete uninstall procedure. After forcibly terminating associated processes earlier, these removals should be safe. However, be mindful if these logs are ever needed for debugging prior to removal.
842-852: Check conditions before installing the API.
This block correctly checks for a valid plugin directory and environment file before the installation continues, ensuring consistent state. Creating a default "env" file if one does not exist helps preserve deployment behavior. Sourcing the user’s environment file and defining base paths is straightforward.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
885-890: Add error handling for flash backup scheduling.While the implementation is correct, it could benefit from error handling for the 'at' command.
Here's a suggested improvement:
if [ -f /var/local/emhttp/var.ini ]; then # the system has fully booted, emhttpd and nginx are running echo "Starting flash backup (if enabled)" - echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null + if ! echo "/etc/rc.d/rc.flash_backup start" | at -M now 2>/dev/null; then + echo "Failed to schedule flash backup start" + # Non-fatal error, continue installation + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(2 hunks)
🔇 Additional comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
327-329: LGTM: Clean API removal implementation.
The cleanup process properly removes all API-related files and directories in the correct order.
| # Install the API (previously in rc.d script) | ||
| flash="/boot/config/plugins/dynamix.my.servers" | ||
| [[ ! -d "${flash}" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | ||
| [[ ! -f "${flash}/env" ]] && echo 'env=production' >"${flash}/env" env=production | ||
| # shellcheck disable=SC1091 | ||
| source "${flash}/env" | ||
| # Install the API to /usr/local/unraid-api | ||
| api_base_directory="/usr/local/unraid-api" | ||
| unraid_binary_path="/usr/local/bin/unraid-api" | ||
| # Ensure installation tgz exists | ||
| [[ ! -f "${flash}/unraid-api.tgz" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | ||
| # Stop old process | ||
| [[ -f "${unraid_binary_path}" ]] && ${unraid_binary_path} stop | ||
| # Install unraid-api | ||
| rm -rf "${api_base_directory}" | ||
| mkdir -p "${api_base_directory}" | ||
| tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1 | ||
| # Copy env file | ||
| cp "${api_base_directory}/.env.${env}" "${api_base_directory}/.env" | ||
| # Copy wc files from flash | ||
| if [ -f "${flash}/webComps/unraid.min.js" ]; then | ||
| rm -rf /usr/local/emhttp/webGui/webComps | ||
| mkdir -p /usr/local/emhttp/webGui/webComps | ||
| cp ${flash}/webComps/* /usr/local/emhttp/webGui/webComps | ||
| else | ||
| # not fatal, previous version of unraid.min.js should still exist in /usr/local/emhttp/webGui/webComps | ||
| echo "Note: ${flash}/webComps/unraid.min.js is missing" | ||
| fi | ||
| cd "${api_base_directory}" && npm link --force | ||
| # bail if expected file does not exist | ||
| [[ ! -f "${api_base_directory}/package.json" ]] && echo "unraid-api install failed" && exit 1 | ||
| # if nginx is running, start the api. if not, it will be started by rc.nginx | ||
| if /etc/rc.d/rc.nginx status &>/dev/null; then | ||
| # Start new process | ||
| ${unraid_binary_path} start | ||
| # Note: do not run another unraid-api command until you see "UNRAID API started successfully!" in syslog | ||
| sleep 3 | ||
| echo "unraid-api installed and started" | ||
| else | ||
| echo "unraid-api installed" | ||
| fi | ||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness of API installation process.
The installation process could be improved in several areas:
- Add error handling for npm link command
- Validate tgz contents before extraction
- Consider cleanup on failure
- Replace magic number in sleep command
Here's a suggested improvement:
cd "${api_base_directory}" && npm link --force
-# bail if expected file does not exist
-[[ ! -f "${api_base_directory}/package.json" ]] && echo "unraid-api install failed" && exit 1
+# Check npm link result and clean up on failure
+if ! npm link --force; then
+ echo "npm link failed"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+
+# Validate installation
+if [[ ! -f "${api_base_directory}/package.json" ]]; then
+ echo "unraid-api install failed"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+
# if nginx is running, start the api. if not, it will be started by rc.nginx
if /etc/rc.d/rc.nginx status &>/dev/null; then
# Start new process
${unraid_binary_path} start
# Note: do not run another unraid-api command until you see "UNRAID API started successfully!" in syslog
- sleep 3
+ # Wait for API to start (adjust timeout as needed)
+ TIMEOUT=30
+ for ((i=0; i<TIMEOUT; i++)); do
+ if grep -q "UNRAID API started successfully!" /var/log/syslog; then
+ break
+ fi
+ sleep 1
+ done
+ if ((i == TIMEOUT)); then
+ echo "Timeout waiting for API to start"
+ exit 1
+ fi
echo "unraid-api installed and started"📝 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.
| # Install the API (previously in rc.d script) | |
| flash="/boot/config/plugins/dynamix.my.servers" | |
| [[ ! -d "${flash}" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | |
| [[ ! -f "${flash}/env" ]] && echo 'env=production' >"${flash}/env" env=production | |
| # shellcheck disable=SC1091 | |
| source "${flash}/env" | |
| # Install the API to /usr/local/unraid-api | |
| api_base_directory="/usr/local/unraid-api" | |
| unraid_binary_path="/usr/local/bin/unraid-api" | |
| # Ensure installation tgz exists | |
| [[ ! -f "${flash}/unraid-api.tgz" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | |
| # Stop old process | |
| [[ -f "${unraid_binary_path}" ]] && ${unraid_binary_path} stop | |
| # Install unraid-api | |
| rm -rf "${api_base_directory}" | |
| mkdir -p "${api_base_directory}" | |
| tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1 | |
| # Copy env file | |
| cp "${api_base_directory}/.env.${env}" "${api_base_directory}/.env" | |
| # Copy wc files from flash | |
| if [ -f "${flash}/webComps/unraid.min.js" ]; then | |
| rm -rf /usr/local/emhttp/webGui/webComps | |
| mkdir -p /usr/local/emhttp/webGui/webComps | |
| cp ${flash}/webComps/* /usr/local/emhttp/webGui/webComps | |
| else | |
| # not fatal, previous version of unraid.min.js should still exist in /usr/local/emhttp/webGui/webComps | |
| echo "Note: ${flash}/webComps/unraid.min.js is missing" | |
| fi | |
| cd "${api_base_directory}" && npm link --force | |
| # bail if expected file does not exist | |
| [[ ! -f "${api_base_directory}/package.json" ]] && echo "unraid-api install failed" && exit 1 | |
| # if nginx is running, start the api. if not, it will be started by rc.nginx | |
| if /etc/rc.d/rc.nginx status &>/dev/null; then | |
| # Start new process | |
| ${unraid_binary_path} start | |
| # Note: do not run another unraid-api command until you see "UNRAID API started successfully!" in syslog | |
| sleep 3 | |
| echo "unraid-api installed and started" | |
| else | |
| echo "unraid-api installed" | |
| fi | |
| # Install the API (previously in rc.d script) | |
| flash="/boot/config/plugins/dynamix.my.servers" | |
| [[ ! -d "${flash}" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | |
| [[ ! -f "${flash}/env" ]] && echo 'env=production' >"${flash}/env" env=production | |
| # shellcheck disable=SC1091 | |
| source "${flash}/env" | |
| # Install the API to /usr/local/unraid-api | |
| api_base_directory="/usr/local/unraid-api" | |
| unraid_binary_path="/usr/local/bin/unraid-api" | |
| # Ensure installation tgz exists | |
| [[ ! -f "${flash}/unraid-api.tgz" ]] && echo "Please reinstall the Unraid Connect plugin" && exit 1 | |
| # Stop old process | |
| [[ -f "${unraid_binary_path}" ]] && ${unraid_binary_path} stop | |
| # Install unraid-api | |
| rm -rf "${api_base_directory}" | |
| mkdir -p "${api_base_directory}" | |
| tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1 | |
| # Copy env file | |
| cp "${api_base_directory}/.env.${env}" "${api_base_directory}/.env" | |
| # Copy wc files from flash | |
| if [ -f "${flash}/webComps/unraid.min.js" ]; then | |
| rm -rf /usr/local/emhttp/webGui/webComps | |
| mkdir -p /usr/local/emhttp/webGui/webComps | |
| cp ${flash}/webComps/* /usr/local/emhttp/webGui/webComps | |
| else | |
| # not fatal, previous version of unraid.min.js should still exist in /usr/local/emhttp/webGui/webComps | |
| echo "Note: ${flash}/webComps/unraid.min.js is missing" | |
| fi | |
| cd "${api_base_directory}" && npm link --force | |
| # Check npm link result and clean up on failure | |
| if ! npm link --force; then | |
| echo "npm link failed" | |
| rm -rf "${api_base_directory}" | |
| exit 1 | |
| fi | |
| # Validate installation | |
| if [[ ! -f "${api_base_directory}/package.json" ]]; then | |
| echo "unraid-api install failed" | |
| rm -rf "${api_base_directory}" | |
| exit 1 | |
| fi | |
| # if nginx is running, start the api. if not, it will be started by rc.nginx | |
| if /etc/rc.d/rc.nginx status &>/dev/null; then | |
| # Start new process | |
| ${unraid_binary_path} start | |
| # Note: do not run another unraid-api command until you see "UNRAID API started successfully!" in syslog | |
| # Wait for API to start (adjust timeout as needed) | |
| TIMEOUT=30 | |
| for ((i=0; i<TIMEOUT; i++)); do | |
| if grep -q "UNRAID API started successfully!" /var/log/syslog; then | |
| break | |
| fi | |
| sleep 1 | |
| done | |
| if ((i == TIMEOUT)); then | |
| echo "Timeout waiting for API to start" | |
| exit 1 | |
| fi | |
| echo "unraid-api installed and started" | |
| else | |
| echo "unraid-api installed" | |
| fi |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
876-883: Improve reliability of flash backup initialization.The flash backup startup process needs better error handling and verification:
- Add error handling for the 'at' command
- Verify flash backup script exists
- Update outdated comment about emhttpd
Here's a suggested improvement:
-# start background process to start flash backup +# Initialize flash backup if system is ready echo if [ -f /var/local/emhttp/var.ini ]; then # the system has fully booted, emhttpd and nginx are running echo "Starting flash backup (if enabled)" - echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null + if [[ -x /etc/rc.d/rc.flash_backup ]]; then + if ! echo "/etc/rc.d/rc.flash_backup start" | at -M now 2>/dev/null; then + echo "Warning: Failed to schedule flash backup start" + fi + else + echo "Warning: Flash backup script not found or not executable" + fi # else - # the system is booting, emhttpd will install the api and start rc.flash_backup. rc.nginx will start the api. + # the system is still booting, rc.nginx will start the api when ready # nothing to do here fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(2 hunks)
🔇 Additional comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
842-875: 🛠️ Refactor suggestion
Enhance error handling and robustness of API installation process.
The API installation process could be improved in several areas:
- Add error handling for tar extraction
- Validate extracted contents before npm link
- Implement cleanup on failure
- Add timeout for process stop
- Consider moving hardcoded paths to variables section
Here's a suggested improvement:
# Install the API (previously in rc.d script)
echo "Extracting and installing the Unraid API"
+# Define paths at the top for better maintainability
+API_INSTALL_PATHS=(
+ "${api_base_directory}/package.json"
+ "${api_base_directory}/.env.${env}"
+)
+
# Stop old process
-[[ -f "${unraid_binary_path}" ]] && ${unraid_binary_path} stop
+if [[ -f "${unraid_binary_path}" ]]; then
+ timeout 30s ${unraid_binary_path} stop
+ if [ $? -eq 124 ]; then
+ echo "Timeout waiting for process to stop"
+ exit 1
+ fi
+fi
# Install unraid-api
rm -rf "${api_base_directory}"
mkdir -p "${api_base_directory}"
-tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1
+
+# Extract with error handling
+if ! tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1; then
+ echo "Failed to extract API files"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+
+# Validate extracted contents
+for path in "${API_INSTALL_PATHS[@]}"; do
+ if [[ ! -f "${path}" ]]; then
+ echo "Missing required file: ${path}"
+ rm -rf "${api_base_directory}"
+ exit 1
+ fi
+done
# Copy env file
cp "${api_base_directory}/.env.${env}" "${api_base_directory}/.env"
-npm link "${api_base_directory}" --force
-# bail if expected file does not exist
-[[ ! -f "${api_base_directory}/package.json" ]] && echo "unraid-api install failed" && exit 1
+# Run npm link with error handling
+if ! npm link "${api_base_directory}" --force; then
+ echo "npm link failed"
+ rm -rf "${api_base_directory}"
+ exit 1
+fiLikely 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: 0
🧹 Nitpick comments (1)
api/scripts/build.mjs (1)
67-67: Consider pinning the GitHub dependency for more reliable builds.While installing directly from a GitHub repository may be convenient, it poses risks for reproducibility if the repository tags or branches change unexpectedly. Pinning a specific commit or tag helps ensure more consistent builds. For example:
- await $`npm install github:unraid/libvirt`; + await $`npm install github:unraid/libvirt#<commit-hash-or-tag>`;
|
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: 0
🧹 Nitpick comments (2)
plugin/plugins/dynamix.unraid.net.plg (2)
327-329: Add error handling for directory and file removal operations.While the removal commands are correct, consider adding error handling to ensure clean uninstallation:
- rm -rf /usr/local/unraid-api - rm -rf /var/log/unraid-api/ - rm -f /var/run/unraid-api.sock + if ! rm -rf /usr/local/unraid-api; then + echo "Warning: Failed to remove /usr/local/unraid-api" + fi + if ! rm -rf /var/log/unraid-api/; then + echo "Warning: Failed to remove /var/log/unraid-api/" + fi + if ! rm -f /var/run/unraid-api.sock; then + echo "Warning: Failed to remove /var/run/unraid-api.sock" + fi
881-895: Improve background process robustness.The background process that waits for system readiness could be improved:
- The timeout value should be configurable
- Add logging for timeout events
- Handle process termination gracefully
-( timeout=30 elapsed=0 +( + # Configuration + WAIT_TIMEOUT=${WAIT_TIMEOUT:-30} # Allow override via environment + elapsed=0 + + cleanup() { + logger "Background process terminated while waiting for var.ini" + exit 1 + } + trap cleanup SIGTERM SIGINT + - while (( elapsed < timeout )); do + while (( elapsed < WAIT_TIMEOUT )); do if [ -f /var/local/emhttp/var.ini ]; then logger "Starting flash backup (if enabled)" echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null logger "Starting Unraid API" - ${unraid_binary_path} start + if ! ${unraid_binary_path} start; then + logger "Failed to start Unraid API" + exit 1 + fi exit 0 fi sleep 1 (( elapsed++ )) done - echo "Timeout waiting for /var/local/emhttp/var.ini" + logger "Timeout after ${WAIT_TIMEOUT}s waiting for /var/local/emhttp/var.ini" + exit 1 ) &
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(3 hunks)
🔇 Additional comments (2)
plugin/plugins/dynamix.unraid.net.plg (2)
875-879: 🛠️ Refactor suggestion
Enhance npm link error handling and validation.
The npm link command needs proper error handling and cleanup on failure:
-npm link "${api_base_directory}" --force
-# Create symlink to unraid-api binary (to allow usage elsewhere)
-ln -sf /usr/local/bin/unraid-api /usr/local/sbin/unraid-api
-# bail if expected file does not exist
-[[ ! -f "${api_base_directory}/package.json" ]] && echo "unraid-api install failed" && exit 1
+if ! npm link "${api_base_directory}" --force; then
+ echo "npm link failed"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+
+# Create symlink to unraid-api binary (to allow usage elsewhere)
+if ! ln -sf /usr/local/bin/unraid-api /usr/local/sbin/unraid-api; then
+ echo "Failed to create symlink"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+
+# Validate installation
+if [[ ! -f "${api_base_directory}/package.json" ]]; then
+ echo "unraid-api install failed: package.json not found"
+ rm -rf "${api_base_directory}"
+ exit 1
+fiLikely invalid or redundant comment.
863-864: 🛠️ Refactor suggestion
Validate tar extraction and handle errors.
The tar extraction lacks error handling and content validation:
-tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1
+if ! tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1; then
+ echo "Failed to extract unraid-api.tgz"
+ rm -rf "${api_base_directory}"
+ exit 1
+fi
+# Validate essential files
+for file in "package.json" ".env.${env}"; do
+ if [[ ! -f "${api_base_directory}/${file}" ]]; then
+ echo "Missing required file: ${file}"
+ rm -rf "${api_base_directory}"
+ exit 1
+ fi
+doneLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
882-896: Improve background process robustness.The background process for starting services could be enhanced:
- The timeout value (30) should be configurable
- Add error handling for the service start commands
- Consider logging the timeout error to syslog
# Start a background process to wait for /var/local/emhttp/var.ini -( timeout=30 elapsed=0 +( + TIMEOUT=${CONNECT_TIMEOUT:-30} # Allow configuration via environment + elapsed=0 while (( elapsed < timeout )); do if [ -f /var/local/emhttp/var.ini ]; then logger "Starting flash backup (if enabled)" - echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null + if ! echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null; then + logger "Failed to schedule flash backup start" + fi logger "Starting Unraid API" - ${unraid_binary_path} start + if ! ${unraid_binary_path} start; then + logger "Failed to start Unraid API" + exit 1 + fi exit 0 fi sleep 1 (( elapsed++ )) done - echo "Timeout waiting for /var/local/emhttp/var.ini" + error_msg="Timeout waiting for /var/local/emhttp/var.ini after ${TIMEOUT} seconds" + echo "${error_msg}" + logger "${error_msg}" ) &
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
plugin/plugins/dynamix.unraid.net.plg(3 hunks)
🔇 Additional comments (2)
plugin/plugins/dynamix.unraid.net.plg (2)
327-329: LGTM: Clean and thorough API cleanup.
The uninstallation cleanup properly removes all API-related artifacts in a logical order.
843-880: Enhance robustness of API installation process.
The installation process could be improved in several areas:
- Add error handling for npm link command
- Validate tgz contents before extraction
- Consider cleanup on failure
- Replace magic number in sleep command
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
| if [ -f "${flash}/webComps/unraid.min.js" ]; then | ||
| rm -rf /usr/local/emhttp/webGui/webComps | ||
| mkdir -p /usr/local/emhttp/webGui/webComps | ||
| cp ${flash}/webComps/* /usr/local/emhttp/webGui/webComps | ||
| else | ||
| # not fatal, previous version of unraid.min.js should still exist in /usr/local/emhttp/webGui/webComps | ||
| echo "Note: ${flash}/webComps/unraid.min.js is missing" | ||
| fi |
There was a problem hiding this comment.
Likely not needed anymore. This was the old location for the previous version of the web components.
There was a problem hiding this comment.
Thanks for the info, deleted.
elibosley
force-pushed
the
feat/remove-rc-script-usage
branch
from
f8fcd41 to
d9c06bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/plugins/dynamix.unraid.net.plg (1)
873-887: Consider improving the background process implementation.The background process implementation could be enhanced:
- The timeout mechanism uses a simple counter which doesn't account for system load
- The process doesn't clean up after timeout
- The error message on timeout could be more informative
( timeout=30 elapsed=0 + trap 'echo "Background process terminated"; exit 1' TERM INT while (( elapsed < timeout )); do if [ -f /var/local/emhttp/var.ini ]; then logger "Starting flash backup (if enabled)" echo "/etc/rc.d/rc.flash_backup start" | at -M now &>/dev/null logger "Starting Unraid API" ${unraid_binary_path} start exit 0 fi sleep 1 (( elapsed++ )) done - echo "Timeout waiting for /var/local/emhttp/var.ini" + logger "Error: Timeout after ${timeout}s waiting for /var/local/emhttp/var.ini" + echo "Error: System might be under heavy load or var.ini generation failed" ) &
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/scripts/build.mjs(1 hunks)plugin/plugins/dynamix.unraid.net.plg(3 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/scripts/build.mjs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Build and Test API
🔇 Additional comments (4)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)
9-9: LGTM! Installation logic moved to plugin file.The installation logic has been correctly moved to the plugin file, simplifying this script's responsibilities.
12-12: LGTM! Uninstallation logic moved to plugin file.The uninstallation logic has been correctly moved to the plugin file, simplifying this script's responsibilities.
plugin/plugins/dynamix.unraid.net.plg (2)
327-329: Verify cleanup of API-related files.The uninstallation process removes API-related files and directories. Let's verify that all paths are being cleaned up correctly.
Also applies to: 374-374
✅ Verification successful
Cleanup of API-related files is comprehensive
The uninstallation process correctly removes all essential runtime files and directories:
- Main installation directory (/usr/local/unraid-api)
- Log files (/var/log/unraid-api/)
- Socket file (/var/run/unraid-api.sock)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no API-related files are left behind after uninstallation # Test: Search for any remaining API-related files echo "Searching for API-related files..." rg -l "unraid-api" # Test: Check for any remaining API-related directories echo "Checking for API-related directories..." fd -t d "unraid-api"Length of output: 3300
843-872: 🛠️ Refactor suggestionEnhance error handling in API installation process.
While the installation logic is well-structured, it could benefit from more robust error handling:
- Add error handling for the tar extraction
- Validate the environment file before copying
- Add error handling for npm link
tar -C "${api_base_directory}" -xzf "${flash}/unraid-api.tgz" --strip 1 +if [ $? -ne 0 ]; then + echo "Failed to extract API package" + exit 1 +fi + # Copy env file +if [ ! -f "${api_base_directory}/.env.${env}" ]; then + echo "Environment file .env.${env} not found" + exit 1 +fi cp "${api_base_directory}/.env.${env}" "${api_base_directory}/.env" -npm link "${api_base_directory}" --force + +if ! npm link "${api_base_directory}" --force; then + echo "npm link failed" + exit 1 +fiLikely invalid or redundant comment.
|
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
Refactor
Chores