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

Fix copy/paste-ability of default URL presented on startup #4103

Merged
merged 2 commits into from Nov 7, 2018
Merged

Fix copy/paste-ability of default URL presented on startup #4103

merged 2 commits into from Nov 7, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 15, 2018

Currently the default URL message given on the console on startup is:

Copy/paste this URL into your browser when you connect for the first time,
to login with a token:
http://(myip.com or 127.0.0.1):8888/?token=8fdc8 ...

This will always need editing to use. Replace brackets/ 'or' part with host IP (e.g. 'myip.com')
to make it copy/pastable again.

@minrk
Copy link
Member

minrk commented Oct 15, 2018

This is the same as #3356, which was reverted because it caused problems. See #3605, #3668, #3703 for why we don't do this, and various discussions about what to do and when.

The latest proposal in #3947 is to display both the hostname and localhost URL in copy-pasteable form, which might be the best so far. Would you like to take a stab at doing that?

@ghost
Copy link
Author

ghost commented Oct 15, 2018

Proposal stabbed at, comments welcome.

@Carreau
Copy link
Member

Carreau commented Oct 21, 2018

Thanks,

Your editor seem to be configured to trim whitespaces, this adds a lot of noise in the diff, and also often is the source of issues when trying to backport Pull-requests / use Git blame, or git-bisect.

We in general are in favor of removing whitespace, but only in proximity of modified code.

How comfortable are you in rebasing without the whitespace changes ? Otherwise we can do that for you.

Currently the default URL message given on the console on startup is:

---
Copy/paste this URL into your browser when you connect for the first time,
    to login with a token:
        http://(myip.com or 127.0.0.1):8888/?token=8fdc8 ...
---

This will always need editing to use. Replace with host IP (e.g. 'myip.com')
to make it copy/pastable again.
Currently the default URL message given on the console on startup is:

---
Copy/paste this URL into your browser when you connect for the first time,
to login with a token:
     http://(myip.com or 127.0.0.1):8888/?token=8fdc8 ...
---

This will always need editing to use. Replace with one host IP (e.g. 'myip.com')
option and one local ip (127.0.0.1) option to make it copy/pastable again.
@ghost
Copy link
Author

ghost commented Oct 22, 2018

Hi, thanks for looking at this PR.

The whitespace changes were in a separate commit for the reasons you quote, but I've just reverted that anyway.

I've also rebased my local branch to the head of master and force-pushed, which I assume was causing the previous merge error. There's been some issues with github's UI/backend whilst this went on, so please let me know if anything looks untoward.

Thanks,

Mark

@Carreau
Copy link
Member

Carreau commented Oct 22, 2018

The whitespace changes were in a separate commit for the reasons you quote, but I've just reverted that anyway.

Actually as the last two commits were the whitespace and faulty one I've hard reseted to HEAD~2 so they are just not there.

I'll wait until github full restoration of service to merge.

@ghost
Copy link
Author

ghost commented Oct 25, 2018

The CI pipeline looks to be stuck on this PR, given that the same code changes passed a few days ago. Can it be restarted?

@ghost
Copy link
Author

ghost commented Nov 1, 2018

Hi @Carreau, just checking in - is there anything I can do to help merge this PR? I don't believe the current CI checks are sane.

@takluyver
Copy link
Member

@Einon generally closing and reopening a pull request will re-run the CI. I'll try that now.

@takluyver takluyver closed this Nov 7, 2018
@takluyver takluyver reopened this Nov 7, 2018
url = url_concat(url, {'token': token})
url = (url_concat(url, {'token': token})
+ '\n or '
+ url_concat(self._url('127.0.0.1'), {'token': token}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've previously been trying to show only the real IP if there's one set, and make the 'or 127.0.0.1' a special case when you're listening on all IPs. This would show the 127.0.0.1 option all the time, even when you're not listening on that IP, and even when it's the same as the first address. But maybe that's better than trying to be clever about it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @takluyver, thanks for the update.
In my experience it would be an unusual system not to be listening on 127.0.0.1, the loopback address. Is that an existing use case for jupyter notebooks?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely possible to listen only on an external-facing interface, though I don't know how many people do it. But even when you do that, it might be e.g. the external network interface of a container, which is mapped to the same port on localhost outside the container. There's no way in general to know what address the server is actually accessible at.

So I'm inclined to merge this, and we can always reconsider it again later.

@AlJohri
Copy link

AlJohri commented Nov 20, 2018

Thanks for this! I was just about to make an issue regarding the copy/pastability.

@Markus92
Copy link

Would it be possible to apply this patch to the 5.7.x branch? It'd allow me to put it into production right away (and make our users very happy!).

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.

None yet

6 participants