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 preflight and delete option in sessions handler #363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

damiles
Copy link

@damiles damiles commented May 24, 2021

When try to call a delete session, /api/sessions/<session_id> we obtain a preflight error and cors. See screenshots for more info:

image

image

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.

@kevin-bates
Copy link
Member

@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)

@lresende
Copy link
Member

lresende commented Mar 2, 2023

I haven't actually tested but did a little research on pre-flight calls and the code looks good.

@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Patch coverage: 87.50% and project coverage change: -0.12 ⚠️

Comparison is base (8058889) 95.78% compared to head (89aac05) 95.66%.

❗ Current head 89aac05 differs from pull request most recent head 442ec70. Consider uploading reports for the commit 442ec70 to get more accurate results

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     
Impacted Files Coverage Δ
kernel_gateway/services/sessions/handlers.py 86.95% <87.50%> (-13.05%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kevin-bates
Copy link
Member

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 2.5.3) so we can look into creating a 3.0 release that uses Jupyter Server rather than notebook for its foundation.

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.

@damiles
Copy link
Author

damiles commented Mar 6, 2023

@kevin-bates Don't worry, I will test it on my own frontend as soon as possible.

@kevin-bates
Copy link
Member

Awesome - thank you @damiles! Please let us know how it goes and we'll cut a release as soon as this PR is merged.

Comment on lines +37 to +45
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()
Copy link
Member

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?

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.

4 participants