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

Prefer IP address over hostname when available #3593

Merged
merged 2 commits into from May 5, 2018

Conversation

Projects
None yet
3 participants
@takluyver
Copy link
Member

takluyver commented May 4, 2018

Following discussion on #3356.

@evandam @cyberwillis please let me know if you think this is a good idea. I haven't encountered the same issues as either of you, so I don't want to merge this without some confirmation that it's actually an improvement!

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented May 4, 2018

And are there any other values for self.ip like '0.0.0.0', which can't be directly used?

@cyberwillis

This comment has been minimized.

Copy link

cyberwillis commented May 4, 2018

To me its working fine with this code.
I replicate your changes locally here and tested:

0.0.0.0 : 8888 => container_name : 8888
' ' : 8888 => localhost : 8888
127.0.0.1:8888 => 127.0.0.1 : 8888
xxx.xxx.xxx.xxx : 8888 => xxx.xxx.xxx.xxx : 8888
'*' : 8888 => container_name : 8888

Thanks

[UPDATED @evandam ]

@cyberwillis
Copy link

cyberwillis left a comment

To me this changes are working great !
I tested all the cases.

@evandam

This comment has been minimized.

Copy link
Contributor

evandam commented May 5, 2018

Looks great to me! Does this handle when you do something like --ip='*'? If I recall it's the same thing as passing 0.0.0.0, too.

@cyberwillis

This comment has been minimized.

Copy link

cyberwillis commented May 5, 2018

Yes, it does. I have tested earlier.

@evandam

This comment has been minimized.

Copy link
Contributor

evandam commented May 5, 2018

Just read that after I asked. I'm all for this!

@cyberwillis

This comment has been minimized.

Copy link

cyberwillis commented May 5, 2018

:)

@takluyver takluyver added this to the 5.5 milestone May 5, 2018

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented May 5, 2018

Thanks both! In it goes.

@takluyver takluyver merged commit 11743f1 into jupyter:master May 5, 2018

4 checks passed

codecov/patch 66.66% of diff hit (target 0%)
Details
codecov/project 74.46% (-0.01%) compared to 5cfba9c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@takluyver takluyver deleted the takluyver:url-prefer-ipaddr branch May 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.