-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Start nginx_proxy as a service #4164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Home assistant is usable once http is available. We don't need to wait until home assistant is ready to start the proxy, we can start nginx_proxy earlier and wait for the home assistant frontend to be ready.
📝 WalkthroughWalkthroughAdds version 3.13.1: updates addon config with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S6 as s6 supervisor
participant Run as nginx run script
participant HA as Home Assistant Core
participant N as nginx
S6->>Run: start
Run->>Run: bashio::core.port -> port
rect rgba(200,230,255,0.35)
note right of Run: Readiness loop (poll homeassistant.local.hass.io:<port>)
Run->>HA: HTTP GET /
HA-->>Run: 200 OK / fail
Run-->>Run: retry (print ".", sleep 0.1s) until success
end
Run->>N: exec nginx start
N-->>S6: running
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nginx_proxy/CHANGELOG.md (1)
3-6
: Clarify and align changelog entry with repo style; expand to capture the behavior changeUse “add-on” (hyphenated) and avoid the “HA” acronym. Also mention the two material changes (startup phase and readiness wait) so downstream readers understand why/what changed.
Apply:
-## 3.13.1 - -- Start the addon earlier so HA is accessible earlier. +## 3.13.1 + +- Start the add-on earlier (startup: services) and wait for Home Assistant to be reachable before starting nginx to reduce 504 errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
nginx_proxy/CHANGELOG.md
(1 hunks)nginx_proxy/config.yaml
(2 hunks)nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md)
: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md)
: - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
nginx_proxy/CHANGELOG.md
nginx_proxy/config.yaml
🔇 Additional comments (2)
nginx_proxy/config.yaml (1)
2-2
: Startup phase change looks right; keep as services“services” starts before Home Assistant and is appropriate for a reverse proxy. No issues with the version bump.
If you adopt the Supervisor API–based readiness check suggested in the run script review, ensure the add-on has access to SUPERVISOR_TOKEN (already provided by hassio_api: true). Optionally add homeassistant_api: true for clarity per docs, though it isn’t strictly required when using the Supervisor proxy. (developers.home-assistant.io)
Also applies to: 16-16
nginx_proxy/rootfs/etc/s6-overlay/s6-rc.d/nginx/run (1)
54-64
: Replace brittle probe (hostname + scheme) with Supervisor proxy; add timeout/backoffReplace the hostname+HTTP probe with a Supervisor-proxied health check (/core/api) using Authorization and a bounded wait; fall back to starting nginx after a timeout.
-bashio::log.info "Waiting for core to be ready..." -port=$(bashio::core.port) -until curl \ - --output /dev/null \ - --silent \ - --fail \ - "http://homeassistant.local.hass.io:$port"; -do - printf '.' - sleep 0.1 -done +bashio::log.info "Waiting for Home Assistant to be ready (Supervisor proxy)..." +timeout=60 +start=$(date +%s) +until curl --silent --fail --connect-timeout 2 --max-time 3 \ + -H "Authorization: Bearer ${SUPERVISOR_TOKEN}" \ + http://supervisor/core/api/config > /dev/null; do + if [ $(( $(date +%s) - start )) -ge $timeout ]; then + bashio::log.warning "Timed out waiting for Home Assistant after ${timeout}s; starting nginx anyway." + break + fi + sleep 0.5 +done +echoVerification: repository search returned no matches for homeassistant.local.hass.io or http://homeassistant:, but SUPERVISOR_TOKEN was not set in this environment — ensure SUPERVISOR_TOKEN is injected at add-on runtime (or retrieve it via the bashio supervisor helper) before merging.
The changelog suggestion from @CodeRabbit was good, I updated it. |
Tip For best results, initiate chat on the files or code changes. @zeehio Thank you for the positive feedback! I'm glad the changelog suggestion was helpful and that you were able to update it accordingly. The changelog entry now clearly communicates the changes and their benefits to users. If you need any further assistance with this PR or have questions about any other aspects of the implementation, feel free to let me know! 🐰✨ |
Before this PR, home assistant starts before the nginx proxy. On my system the add-on takes a while to start.
I am not sure when all addons with "startup: application" are started, is it maybe after all integrations have started to load or maybe later?
Anyway, after this PR, the nginx_proxy addon starts first, and waits for the frontend to be ready (We wait to prevent 504 errors from nginx).
This makes the frontend available earlier to the user, at least on my system and my experience.
Summary by CodeRabbit