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

Add host option to serve on other than localhost #79

Closed
wants to merge 2 commits into from

Conversation

SndR85
Copy link
Contributor

@SndR85 SndR85 commented Jan 3, 2017

I wanted an option to do serve on a other IP-address that localhost. This change make it possible to add -h / --host option te set a different IP-adress the webserver will listen on.

I have made the change quite simple. My ruby knowledge is not great and I have not tested the change fully. I could not get the spec-test working and have not changed it. I tested that it was running on 0.0.0.0 when I run reveal-ck serve -h 0.0.0.0 and it did. Also the banner is printing the current address specified by -h.

@SndR85 SndR85 changed the title Add host option to serve, to serve on other than localhost Add host option to serve on other than localhost Jan 3, 2017
@SndR85 SndR85 force-pushed the feature/serve-host-option branch from 6059ac5 to a803cb9 Compare January 3, 2017 22:17
@kbrock
Copy link
Contributor

kbrock commented Jan 3, 2017

I had thought that reveal-ck serve was more of a development thing. So serving to non localhost seems odd to me. (but just because it doesn't meet my use case doesn't mean anything really)

Would it make sense to just serve everything off of 0.0.0.0 and not add this flag at all?
I mean, fewer flags in my book is better. And if this is not intended to be run on a production box, then opening up to loopback or to a remote seems good.

@jedcn
Copy link
Owner

jedcn commented Jan 4, 2017

Interesting idea! What's your motivation? Do you want to serve on 0.0.0.0 so that people cannot accidentally view your presentation while you're working on it? Or is there another reason?

What value of --host do you anticipate using?

...

No worries about the failing build. This thing is picky. I'll take a look later on tonight.

@kbrock
Copy link
Contributor

kbrock commented Jan 4, 2017

@jedcn actually it is the opposite. I think the old default value was localhost - saying no one can view it except via the loopback interface - so it will only work on the machine at network address localhost / 127.0.0.1.

A value of 0.0.0.0 says to bind on all network interfaces, so anyone can view it, even from remote hosts.

Rails in development mode has you type -b 0.0.0.0 to allow other machines to connect.

@SndR85
Copy link
Contributor Author

SndR85 commented Jan 4, 2017

The use case for me was to see if I can run reveal-ck in Docker. And to expose serve te the world outside the container, you need to set 0.0.0.0 as listen-address, so that anybody from outside the container can connect to reveal-ck.

And yes, 127.0.0.1 (or localhost) is intended to be connected only local, not from the outside. 0.0.0.0 is world accessable. See also: https://en.wikipedia.org/wiki/0.0.0.0.

@jedcn
Copy link
Owner

jedcn commented Jan 4, 2017

Okay, cool. I'm convinced. I'll either:

  1. add --host, or
  2. default to 0.0.0.0, or
  3. do both

Should be out in a few days, the end of the coming weekend at the latest.

Speak up if you need it sooner.

Thanks for the PR @scornelissen85.

Thanks for chiming in @kbrock.

@jedcn jedcn mentioned this pull request Jan 5, 2017
@jedcn
Copy link
Owner

jedcn commented Jan 5, 2017

I re-used the commits from this PR and opened up #81.

I added some administrative commits on top of yours.

I'll do a bit of testing to make sure things are working as expected and we should be good to go.

Once I merge #81 in, I'll close this one (#79) but your code and your authorship of it will still be used.

Thanks!

@jedcn
Copy link
Owner

jedcn commented Jan 7, 2017

Pull #81 has been merged in and released. The released version is 3.5.0:

https://rubygems.org/gems/reveal-ck

Please check it out and see if it works for your use case: https://rubygems.org/gems/reveal-ck/versions/3.5.0

I'm going to shortly be releasing version 3.5.1 which will change around a bunch of gems. This could impact you-- that's why I released your requested change first.

If you don't get around to testing for a few days, you'll grab version 3.5.1 and if that doesn't work (say, due to gems) please try explicitly using 3.5.0.

I'm going to close this PR now. Thanks again.

@jedcn jedcn closed this Jan 7, 2017
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