feat: checking if port is used and adding new flag to change the default port #15
Conversation
src/utils/server.cr
Outdated
| terminal.print "#{COG} Would you like to to use port #{new_port} instead? (Y/n)\n" | ||
|
|
||
| use_new_port = gets | ||
| if use_new_port === "y" || use_new_port === "Y" |
There was a problem hiding this comment.
Maybe, a bit more uniform:
use_new_port.downcase == "y"
src/utils/server.cr
Outdated
| server = | ||
| HTTP::Server.new(config.host_binding, port, config.handlers) | ||
| terminal.print "#{COG} Development server started on http://#{config.host_binding}:#{port}/\n" | ||
| else |
There was a problem hiding this comment.
I think it should fail in this case. Having blocking terminal IO on a webserver isn't good practice since it looks like the server is running while in reality it's waiting for user input.
So we could at least check if the terminal is a tty first and fail otherwise. Something like STDIN.tty? should do the trick.
There was a problem hiding this comment.
I have tested it so not sure what you mean by saying it should fail. But if it something important I am happy to change it just point me to the direction how it should be implemented.
There was a problem hiding this comment.
Yes @manveru is right, someone might use mint start as production server for whatever reason and suppressing the output like mint start > /dev/null would cause it to behave like it's running while in reality it's waiting for user input.
This should be the branch of elseif STDIN.tty? and then the else branch would just exit with a nice message and a code of 1.
There was a problem hiding this comment.
Now I understand what are you talking about, going to add this today.
src/commands/start.cr
Outdated
| long: port, | ||
| short: p, | ||
| default: 3000_i32, | ||
| required: false |
There was a problem hiding this comment.
Also would be awesome if it respected the PORT and HOST environment variables.
There was a problem hiding this comment.
Going to add this soon.
|
What I mean is that the server might not be running in your terminal but
instead as a daemon in systemd, runit, docker, etc. In that case there is
no input from the user. The user won't even see the question, it'll just
not serve anything.
…On Tue, 29 May 2018, 19:08 Domantas, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/utils/server.cr
<#15 (comment)>:
> config = Kemal.config
config.logger = Logger.new
config.setup
- server =
- HTTP::Server.new(config.host_binding, config.port, config.handlers)
+ if port_open?(config.host_binding, port)
+ server =
+ HTTP::Server.new(config.host_binding, port, config.handlers)
+ terminal.print "#{COG} Development server started on http://#{config.host_binding}:#{port}/\n"
+ else
I have tested it so not sure what you mean by saying it should fail. But
if it something important I am happy to change it just point me to the
direction how it should be implemented.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#15 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAANs0ugwd9oLcr3sHbrLVLTj1AkxIJ3ks5t3YCFgaJpZM4URZZa>
.
|
| else | ||
| terminal.print "#{COG} Exiting...\n" | ||
| end | ||
| else |
There was a problem hiding this comment.
How failures are handled in mint is by raising an instance of error https://github.com/mint-lang/mint/tree/master/src/errors which the command handles https://github.com/mint-lang/mint/blob/master/src/commands/command.cr#L43 by printing it's message and exiting with code 1.
Would be nice to have it this way, but It can stay as is, it just needs to exit 1 instead of returning 1.
Feature
Added
--portand-pflags so it would be easier to change the default port. Also added checking if a port is already used by a different application and in that case server will try to choose an open port.