Skip to content
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

Add devcontainer and configure the add-on to support ingress #47

Merged
merged 5 commits into from
Sep 3, 2023

Conversation

siku2
Copy link
Contributor

@siku2 siku2 commented Aug 24, 2023

Blocked by: davidusb-geek/emhass#106

This PR activates ingress for the add-on based on the changes made in the aforementioned PR.
It also adds a devcontainer specifically designed for HA add-on development.

Additionally, the following changes are made:

  • No longer expose port 5000 by default since this is now covered by the ingress
  • Clean up the YAML file (you can thank my formatter for that)

@purcell-lab
Copy link
Contributor

Thanks for your work on this a welcome addition.

  • No longer expose port 5000 by default since this is now covered by the ingress

I don't think you should completely close port 5000 as that is also used for the REST API to control EMHASS., day-ahead, publish, mpc_optim including the injection of data via the REST payload.

@siku2
Copy link
Contributor Author

siku2 commented Aug 25, 2023

Thanks for your work on this a welcome addition.

  • No longer expose port 5000 by default since this is now covered by the ingress

I don't think you should completely close port 5000 as that is also used for the REST API to control EMHASS., day-ahead, publish, mpc_optim including the injection of data via the REST payload.

Good point. Makes sense to leave it open for the time being.

@davidusb-geek
Copy link
Owner

Great contribution, thank you!
Yes a port should be kept open for the REST API.
Could you please revert that part?
Otherwise it would be nice if the add-on user could choose whatever port number it wants for the REST API, I never managed to get that to work. Do you know how to make this possible?

@siku2
Copy link
Contributor Author

siku2 commented Aug 25, 2023

@davidusb-geek done.

Otherwise it would be nice if the add-on user could choose whatever port number it wants for the REST API, I never managed to get that to work. Do you know how to make this possible?

I don't exactly follow what you mean. Are you saying that if the user changes the port mapping inside of the add-on configuration the web server stops working?
I'm happy to look into it, I just don't fully understand the problem yet.

@davidusb-geek
Copy link
Owner

Are you saying that if the user changes the port mapping inside of the add-on configuration the web server stops working?

Yes, but the web server doesn't stop, it is just always bounded to 5000. At least this was the behavior the last time I tested this some time ago.

@siku2
Copy link
Contributor Author

siku2 commented Aug 28, 2023

That sounds like what's supposed to happen. The web server inside of an add-on should use a static port that cannot be changed (like 5000 in this case).
The user can re-assign this port in the add-on config (or disable the port altogether) and this is handled purely by the Supervisor.
The Supervisor will take care of routing the newly assigned port from the outside world to the add-on's internal port.

Btw, the beauty of this system is that even if you change the external port binding you can still make use of ingress. Since all of this routing is handled by the Supervisor, the ingress can bypass any of it and always access port 5000 directly.

@davidusb-geek davidusb-geek merged commit f07ed2f into davidusb-geek:main Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants