Add support for creating terminals via GET #5813
Conversation
|
Thanks for looking into this @kevin-bates 🙏! Could it be that, instead of returning 409, it returns a 302 with Location |
|
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) |
There was a problem hiding this comment.
If the term_name isn't set (i.e. they visit /new), can we have it create a new named one?
There was a problem hiding this comment.
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.
notebook/terminal/handlers.py
Outdated
| model = self.terminal_manager.create() | ||
| term_name = model['name'] | ||
| new_path = self.request.path.replace("terminals/new", "terminals/" + term_name) | ||
| self.redirect(new_path) |
There was a problem hiding this comment.
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).
|
I'm going to try rebuilding the PR, as it doesn't seem like your work should be causing the failure AppVeyor is reporting. |
|
Thank you. I'm also working on a commit to address your last comment - which does clean things up a bit. |
|
Hmm - odd. AppVeyor is successful in my fork - except for one of the commits - but not the one that failed above: 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! |
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), | |||
|
Thank you so much @kevin-bates! |
This pull request adds back support for creating new terminal sessions via an HTTP
GETrequest against/terminals/new/<name>. Upon its creation, the user is redirected to/terminals/<name>. Attempts to issue aGETrequest to/terminals/new/<name>when terminal<name>is already created will result in a409(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