search

Akkudoktor-EOS / EOS

Other
11 stars 5 forks source link

fix: make port configurable via env #23

Closed dsiebel closed 1 day ago

dsiebel commented 2 days ago

At least on my machine, using port 5000 doesn't work, because of another service already using this port. Since the flask port is already configurable via FLASK_RUN_PORT, this configuration could be exposed to the user as well.

Alternative, not so serious suggestion for the port number: 8503, that spells "EOSB" backwards. The "8" is just there to put it in the range of HTTP ports and because allocating anything < 1024 is not advised.

danimo commented 2 days ago

Looks good. I'd go for 3000 (see fix/defaultbranch) or 8000 by default. Not hard-to-remember random ports.

Edit: On second thought: What really bugs me is that we only change the default port in the docker setup. It will be difficult do document that consitently. I don't mind the port really.

dsiebel commented 2 days ago

Looks good. I'd go for 3000 (see fix/defaultbranch) or 8000 by default. Not hard-to-remember random ports.

As I mentioned in #25, port 3000 is the default for Grafana, Express (NodeJS), MongoDB and more. Hence my suggestion to use 8503. I am not emotionally attached to any port number so I changed it (and the default) to 3000 (f5ee200).

EDIT: since I touched flask_server.py, my IDE went ahead and stripped all trailing whitespaces. Hope that's okay, if not, I can revert that.

danimo commented 2 days ago

As mentioned in my edit, all I care for is consistency. Let's go for your port. I retract my port 3000 idea. Sorry for the confusion.

dsiebel commented 1 day ago

As mentioned in my edit, all I care for is consistency. Let's go for your port. I retract my port 3000 idea. Sorry for the confusion.

No worries. Changes pushed.

Btw: have you considered using Squash Commits / Squash Merge to keep the git history a bit cleaner? Looking through it, there seems to be a mix of styles; some messages use conventional commits, some don't. With Squash Merge, merging a PR will only create one commit on main, using the PR title and body as message. Makes it easier to enforce / encourage unified commit messages.

danimo commented 1 day ago

@drbacke Note, mit diesem Merge ändert sich der Default-Port!