fix(api): update deploy-dev script & add ready signal to unraid-api start #1023
Conversation
prior to this, pm2 would mark unraid-api as started as soon as the node process started. The Nest server wouldn't finish booting until later. This meant unraid-api could "start", but the webgui wouldn't be fully functional because it couldn't connect to the api yet. This was a sucky user & dev experience.
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces configuration and deployment enhancements for the Unraid API project. The changes focus on improving application startup behavior by adding readiness and timeout configurations in the ecosystem config, updating the deployment script to handle build processes more robustly, and modifying the main application entry point to signal readiness to the parent process. These modifications streamline the deployment and startup workflow of the application. Changes
Possibly related PRs
Suggested reviewers
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 (1)
api/scripts/deploy-dev.sh (1)
Line range hint
26-64: Consider cleaning old files before deploymentTo prevent stale files from causing issues, consider cleaning the remote directory before deploying new files.
Add remote cleanup before rsync:
# Replace the value inside the rsync command with the user's input +ssh root@"${server_name}" "rm -rf /usr/local/unraid-api/dist/*" rsync_command="rsync -avz -e ssh $source_directory root@${server_name}:/usr/local/unraid-api"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
api/ecosystem.config.json(1 hunks)api/justfile(1 hunks)api/scripts/deploy-dev.sh(2 hunks)api/src/unraid-api/main.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build and Test API
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
api/justfile (1)
11-20: Well-structured command additions!The new commands provide a clear and convenient way to build and deploy the application. The comments effectively document each command's purpose.
api/src/unraid-api/main.ts (1)
51-53: Correct implementation of ready signal!The ready signal is sent at the right time (after server is listening) and with proper parent process check. This ensures pm2 only marks the service as ready after the Nest server is fully operational.
api/ecosystem.config.json (1)
10-11: Verify if 30s listen_timeout is sufficientWhile the wait_ready configuration correctly enables the ready signal feature, please verify if the 30-second listen_timeout is sufficient for all deployment scenarios, especially on slower systems or during high load.
Consider monitoring server startup times in various environments to ensure this timeout is adequate:
✅ Verification successful
30s listen_timeout is appropriate
The 30-second timeout provides a generous safety margin for server startup, which typically completes in a few seconds. The codebase shows a clean initialization sequence without heavy tasks, making this timeout sufficient even for slower systems or high-load scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check pm2 logs for startup duration patterns # Note: This helps identify if 30s is enough time for the server to start ssh root@<unraid-server> "pm2 logs unraid-api --lines 1000" | grep -E "Starting Nest Server|Nest Server is now listening" | awk ' /Starting Nest Server/ {start=$1 " " $2} /Nest Server is now listening/ { end=$1 " " $2 cmd="date -d\"" end "\" +%s.%N" cmd | getline end_ts close(cmd) cmd="date -d\"" start "\" +%s.%N" cmd | getline start_ts close(cmd) printf "Startup took %.2f seconds\n", end_ts - start_ts }'Length of output: 510
Script:
#!/bin/bash # Search for main.ts files and their content fd main.ts --type f --exec cat {} \; # Search for any startup timing or timeout related code rg -i "startup|bootstrap|listen.*time|timeout" --type ts -A 2 -B 2Length of output: 39523
api/scripts/deploy-dev.sh (1)
26-26: Robust build process implementation!The script now correctly handles the build process with proper error checking and exit codes.
Also applies to: 29-34
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
elibosley
left a comment
elibosley
left a comment
There was a problem hiding this comment.
LGTM - just add a comment to the process.send block and merge away!
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
prior to this, pm2 would mark unraid-api as started as soon as the node process started. The Nest server wouldn't finish booting until later. This meant unraid-api could "start", but the webgui wouldn't be fully functional because it couldn't connect to the api yet. This was a sucky user & dev experience.
Summary by CodeRabbit
Configuration Updates
Development Workflow
Performance Improvements