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

Do not hardcode port in config.ru #69

Closed
wants to merge 1 commit into from
Closed

Conversation

ntalbott
Copy link
Contributor

@ntalbott ntalbott commented Apr 6, 2012

Proposed solution for #49, just removes the hard coded port in config.ru.

@bmaland
Copy link
Contributor

bmaland commented Apr 6, 2012

Whats the point of duplicating 108ccc1 in #65 ..? My pull request also runs on port 4000 by default, and fixes serve's inability to run on Heroku Cedar.

@ntalbott
Copy link
Contributor Author

ntalbott commented Apr 6, 2012

@bmaland, I actually created this before I saw your more comprehensive patch. That said, the question in this thread has to be answered either way, as removing the hard coding of port 4000 in config.ru does change functionality and I need to understand the desired behavior before I can determine whether that's OK.

@ntalbott ntalbott mentioned this pull request Apr 6, 2012
@bmaland bmaland mentioned this pull request Apr 6, 2012
@bmaland
Copy link
Contributor

bmaland commented Apr 6, 2012

I think the intended logic is port 3000 for Rails apps, and port 4000 for everything else except when overridden by the command line flag (or ENV["PORT"] for Heroku).

@jlong
Copy link
Owner

jlong commented Apr 8, 2012

Yup that's correct. Any other issues @ntalbott?

@jlong
Copy link
Owner

jlong commented Apr 8, 2012

Mmmm. So the problem with this is that regular Rack apps currently started on 3000 with Serve. The port override in config.ru makes generated Serve apps startup on port 4000 which matches the existing command line functionality when using Serve in "unstructured" mode.

There's not an easy way to detect Serve rack apps, so I'm not sure of a good solution.

@ghost ghost assigned ntalbott Apr 12, 2012
@ntalbott
Copy link
Contributor Author

After looking deeply at Rack, it seems that it simply isn't possible to achieve the ability to specify a port in config.ru and allow that port to be overridden using the command line. This is due to the way Rack/rackup process options, and wouldn't be a simple fix on that side. Thus I'm going to punt on this and say that if one wants to override the port that's in config.ru, they have to delete the comment that forces the app onto 4000.

@ntalbott ntalbott closed this Apr 12, 2012
@jlong
Copy link
Owner

jlong commented Apr 12, 2012

Agreed.

@bmaland
Copy link
Contributor

bmaland commented Apr 13, 2012

Right, if this is the intended behavior then the docs and the output from "serve --help" would have to be updated, since the functionality described there simply doesn't work.

@ntalbott
Copy link
Contributor Author

Hmmm, you mean in terms of the --port option? Documenting this nuance is going to be tricky - any ideas @jlong?

@bmaland
Copy link
Contributor

bmaland commented Apr 13, 2012

Yeah, both serve --help and http://get-serve.com/documentation/usage are currently misleading about this feature.

I'm still not sure why breaking the functionality of the serve executable is preferred over having config.ru have a port set by default - isn't the most common use case to run serve from the command line?

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