-
Notifications
You must be signed in to change notification settings - Fork 132
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 preflight and delete option in sessions handler #363
base: main
Are you sure you want to change the base?
Conversation
@blink1073 or @lresende, I'm hoping one of you could take a look at this PR. I just don't know enough about this aspect of the stack to know if this introduces any issues or not - sorry about that. (Thank you) |
I haven't actually tested but did a little research on pre-flight calls and the code looks good. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
- Coverage 95.78% 95.66% -0.12%
==========================================
Files 31 31
Lines 2252 2260 +8
==========================================
+ Hits 2157 2162 +5
- Misses 95 98 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Hi @damiles - I know it's been "forever" since you opened this PR and we apologize for not being more active on this repository. I'd like to get this PR merged (and released via a patch release Are you able to try out the changes we've been discussing that I just pushed? Since the notebook/jupyter server gateway client doesn't use the Session API when interacting with the Gateway server, I suspect you must have your own frontend that this PR is addressing. |
@kevin-bates Don't worry, I will test it on my own frontend as soon as possible. |
Awesome - thank you @damiles! Please let us know how it goes and we'll cut a release as soon as this PR is merged. |
def set_default_headers(self): | ||
self.set_header('Content-Type', 'application/json') | ||
self.set_header('Access-Control-Allow-Origin', self.settings['kg_allow_origin']) | ||
self.set_header('Access-Control-Allow-Headers', self.settings['kg_allow_headers']) | ||
self.set_header('Access-Control-Allow-Methods', self.settings['kg_allow_methods']) | ||
|
||
def options(self, **kwargs): | ||
"""Method for properly handling CORS pre-flight""" | ||
self.finish() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After working on #384, I'm curious why the existing CORSMixin
isn't already sufficient, since it adds everything that is listed here, except Content-Type
.
Are the bases
of the else
statement below not doing what is intended?
When try to call a delete session, /api/sessions/<session_id> we obtain a preflight error and cors. See screenshots for more info:
I solve adding a new handler and allowing options call for preflight and adding delete options in headers. (Maybe is not all correct for integration because I don't read full kernel_gateway code) But this works fine and solve the issue.
This issue looks like appears in #297 too.