Update deploy scripts

[lxm7]

No description provided.

[claude]

PR Review: Update deploy scripts

Summary

This PR improves the deployment process by addressing DNS resolution issues when container IPs change during rolling deployments. The changes add dynamic DNS resolution to nginx and ensure nginx reloads after app container updates.

---

Code Quality & Best Practices

✅ Positive:

• Good use of inline comments explaining the technical rationale (DNS resolver with 10s TTL)
• The deployment script already follows best practices with set -exo pipefail
• Rolling deployment strategy (scale to 2, then back to 1) minimizes downtime

⚠️ Suggestions:

1. nginx/production.conf:71 - Consider making the TTL configurable via environment variable
2. scripts/production.sh:25 - The docker exec command needs error handling

---

Potential Bugs & Issues

🔴 Critical:

1. scripts/production.sh:20 - The 15-second sleep may not be sufficient. The app healthcheck has interval: 5s, retries: 10, start_period: 15s which means a container could take up to 65 seconds to be considered healthy. Consider waiting for actual health status instead of a fixed sleep.

2. Race condition risk: Between lines 21 and 25, there is a window where old containers may be terminated before nginx reloads, leaving nginx resolving to dead IPs. Consider reloading nginx BEFORE scaling down.

⚠️ Moderate:

3. scripts/production.sh:25 - If nginx reload fails, the deployment continues without alerting. This could leave nginx proxying to old/dead containers.

---

Performance Considerations

✅ Good:

• The valid=10s TTL is a reasonable balance between DNS query overhead and IP change detection
• The $upstream variable pattern forces nginx to re-resolve DNS on each request (when cache expires)

💡 Optimization:

• Consider using proxy_next_upstream directives to automatically retry failed requests to other healthy containers

---

Security Concerns

✅ No major security issues, but some observations:

1. nginx/production.conf:71 - ipv6=off disables IPv6 resolution. This is fine for Docker internal DNS but worth documenting why.

2. nginx/production.conf:65 - Having both SSL (443) and non-SSL (80) listeners in the same server block is unusual. Typically you would want HTTP to redirect to HTTPS. Current config serves the same content over both protocols.

---

Test Coverage

❌ Missing:

• No automated tests for the deployment script
• No validation that nginx actually reloaded successfully
• No rollback mechanism if deployment fails

---

Verdict

The changes address a real issue with container IP resolution during deployments. The approach is sound, but timing and error handling need improvement to make the deployment more robust.

Recommendation: Request changes to address the critical timing issue and add error handling before merging.

---

Suggested Priority Fixes:

1. 🔴 Replace fixed sleep with actual health check waiting
2. 🔴 Add error handling for nginx reload
3. 🟡 Reload nginx before scaling down to avoid race condition
4. 🟡 Add post-deployment validation

Let me know if you would like help implementing any of these suggestions!