-
-
Notifications
You must be signed in to change notification settings - Fork 803
enhance(backend): allow disabling HTTP port completely #911
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
Signed-off-by: Yumechi <yume@yumechi.jp>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #911 +/- ##
==========================================
- Coverage 78.70% 78.51% -0.19%
==========================================
Files 57 57
Lines 2517 2523 +6
==========================================
Hits 1981 1981
- Misses 397 403 +6
Partials 139 139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| hasListener := false | ||
|
|
||
| if conf.Server.Port >= 0 { |
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.
Could this instead be a enabled setting similar to the one for TLS? Should be default true.
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.
I guess so, but it would be called GOTIFY_SERVER_ENABLED=false which semantically sounds funny.
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.
GOTIFY_SERVER_ENABLED=false it really looks confusing... Some may even think that "huh? My server disabled? :p"
But we can use like
GOTIFY_PLAIN_SERVER_ENABLED=false
Or
GOTIFY_HTTP_SERVER_ENABLED=false
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.
What I am trying to say is the name of these keys are tied to the yaml hierarchy by the configor, so if we add a key side by side with the current "port" key it would be called GOTIFY_SERVER_SOMETHING, which I don't like.
But I guess we can use GOTIFY_SERVER_DISABLE_HTTP or GOTIFY_SERVER_HTTP_FUNC=on/off/redirect , both should be good options forward
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.
I'm okay with GOTIFY_SERVER_DISABLE_HTTP but I'm questioning if we really need this functionality or can prevent the problems via a different solution. See #910 (comment)
Resolves #910