-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
bugfix: fix attempting to bind to invalid https port #120
Conversation
you are right, i figured out the problem at https://github.com/ije/rex/blob/0634e671c27790e45a9c151d28f95b1404f06df2/rex.go#L37, i non-dev mode the autotls is enabled. |
thanks, look great to me 🙏 |
server/server.go
Outdated
TLS: rex.TLSConfig{ | ||
} | ||
if httpsPort > 0 { | ||
serverConfig.TLS = rex.TLSConfig{ | ||
Port: uint16(httpsPort), | ||
AutoTLS: rex.AutoTLSConfig{ | ||
AcceptTOS: !isDev, |
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.
suggestion: AcceptTOS: httpsPort > 0 && !isDev
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.
can make the code looks better
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 kind of confused about the business logic regarding AutoTSL
, AcceptTOS
, and TSL
. I would expect AutoTSL
is optional, is it not?
Ok so now that you pointed to me to that code, I see you are using My ideal behavior is that esm.sh has the 443 default, and then I can just set it to |
i don't need to change. when |
What is your intent here |
i mean we should disable the autotls when the https port is zero or in development mode |
866e7c1
to
e8f6461
Compare
@ije thanks, makes sense now, took your suggestion and tested in my deployed env. Works. PR updated. |
amazing, thanks @jimisaacs |
I'm not sure if there's something I'm doing wrong, but when I don't configure this (I do not want https support at all as we do our own TLS termination), it was still failing if the TLS config was passed in.