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 support for creating terminals via GET #5813

Merged
merged 4 commits into from Oct 19, 2020

Conversation

kevin-bates
Copy link
Member

This pull request adds back support for creating new terminal sessions via an HTTP GET request against /terminals/new/<name>. Upon its creation, the user is redirected to /terminals/<name>. Attempts to issue a GET request to /terminals/new/<name> when terminal <name> is already created will result in a 409 (Conflict) status code.

I based these changes on the comments provided by @rgbkrk and @blink1073 on #4180.

Although this feature was previously "hidden", it sounds like it was quite useful and I believe this should be considered somewhat of a regression issue. That said, this "fix" results in a change in user behavior, but I'd prefer we allow for that since the feature was not explicit previously.

This seems to address the issue, but this is relatively unfamiliar territory for me, so I'm hoping others can take it for a spin and/or let me know what else should be done.

Resolves #5790

@santiagobasulto
Copy link

Thanks for looking into this @kevin-bates 🙏! Could it be that, instead of returning 409, it returns a 302 with Location /terminals/<name>. In our case, we just put the link to the terminal in Markdown (hardcoded we could say), so for our students it'd work on the first try, but not after that.

@kevin-bates
Copy link
Member Author

Good point. This improves compatibility with the previous functionality. I've pushed an update.

"""Renders a new terminal interface using the named argument."""
@web.authenticated
def get(self, term_name):
new_path = self.request.path.replace("new/{}".format(term_name), term_name)
Copy link
Member

Choose a reason for hiding this comment

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

If the term_name isn't set (i.e. they visit /new), can we have it create a new named one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I added a commit supporting this. This does preclude someone from creating their own named terminal of 'new' (via GET /terminals/new/new), but that's probably okay.

model = self.terminal_manager.create()
term_name = model['name']
new_path = self.request.path.replace("terminals/new", "terminals/" + term_name)
self.redirect(new_path)
Copy link
Member

Choose a reason for hiding this comment

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

Good approach. It might be good to make this an explicit path in the handlers above with (ujoin(base_url, r"/terminals/new"), ReallyActuallyNewTerminalHandler),, noting that you may have to make it the first entry in the list (and figuring out a better name for that).

@rgbkrk
Copy link
Member

rgbkrk commented Oct 15, 2020

I'm going to try rebuilding the PR, as it doesn't seem like your work should be causing the failure AppVeyor is reporting.

@kevin-bates
Copy link
Member Author

Thank you. I'm also working on a commit to address your last comment - which does clean things up a bit.

@kevin-bates
Copy link
Member Author

Hmm - odd. AppVeyor is successful in my fork - except for one of the commits - but not the one that failed above:
image

I'm not seeing any obvious side effects that I may have introduced as the two additional tests ensure all terminals have been stopped - but it is Windows!

cmd-ntrf added a commit to ComputeCanada/puppet-jupyterhub that referenced this pull request Oct 19, 2020
A regression was introduced in notebook 6.1.0 where it is no
longer possible to start a terminal by simply going to the url
"/terminals/1". A fix is currently underway, but until a new
version of notebook is released, we will settle for 6.0.3.

Related issue:
jupyter/notebook#5790

PR fixing this issue:
jupyter/notebook#5813
@@ -42,6 +42,8 @@ def initialize(nb_app):
terminal_manager.log = nb_app.log
base_url = nb_app.web_app.settings['base_url']
handlers = [
(ujoin(base_url, r"/terminals/new"), NamedTerminalHandler),
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@rgbkrk rgbkrk merged commit 87aa278 into jupyter:master Oct 19, 2020
@rgbkrk
Copy link
Member

rgbkrk commented Oct 19, 2020

Thank you so much @kevin-bates!

@kevin-bates kevin-bates deleted the http-new-terminal branch January 29, 2021 23:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starting with 6.1, I can't start terminals directly with a GET request
3 participants