nginx_proxy: Set nginx proxy to start in services phase#4699
Conversation
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
👮 Files not reviewed due to content moderation or server errors (2)
📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
It just feels wrong to start the proxy before the backend service is up, no? I'd say that your installation might suffer from the same issue discussed in #4164, where a single integration just delays the whole system startup, preventing the system from entering What probably would be ideal to have a startup setting which is tied to the http integration specifically being loaded 🤔 |
|
Ok I've checked how Core not being running is handled, and NGINX just raises a 502 Bad Gateway, I guess not the worst outcome. I think what I am more concerned is missconfiguration: If users set a port for the NGINX proxy which Core uses as its port, then you end up with a NGINX proxy running and occuping a port and no chance to reconfigure the NGINX proxy 😰 |
|
This came up in discussion with @edenhaus. One additional concern here is that if we make the App load before Core, it is very easy to accidentally create port conflicts. E.g. with the change to default port 80 (home-assistant/core#174278), and if the user simply mindlessly installs NGINX proxy with With the current startup order, this can't happen since Core claims the port first. On the other hand, since with the 5 minute time which auto-reverts a new HTTP config (home-assistant/core#174428), the NGINX proxy might not be started on time. Hm, actually, on the other hand, config changes only happen at runtime, not startup time, where we leave the NGINX proxy running. So I think this is a non-issue. Anyhow, to conclude: I think making NGINX proxy |
Hi,
I would like to propose changing the startup phase of the nginx_proxy to 'services' as in other similar projects (e.g. https://github.com/hassio-addons/addon-nginx-proxy-manager/blob/main/proxy-manager/config.yaml#L9). There was already an attempt at this (#4164), but I want to try again with a cleaner pull request. Because in my mind, it makes more sense that way and makes the system as a whole feel more responsive when starting up.
I would greatly appreciate it if this could be merged, or let me know your thoughts and/or suggestions. Thanks for what you do!
Summary by CodeRabbit