-
Notifications
You must be signed in to change notification settings - Fork 315
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
[Notebook port 4835] Add UNIX socket support to notebook server #525
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
- Coverage 76.97% 76.31% -0.67%
==========================================
Files 107 110 +3
Lines 9496 9809 +313
Branches 1031 1067 +36
==========================================
+ Hits 7310 7486 +176
- Misses 1814 1943 +129
- Partials 372 380 +8 ☔ View full report in Codecov by Sentry. |
I went ahead and dismissed the two remaining codeQL issues since they were only in test code (side effect stuff). |
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.
Thanks, @jtpio! 🚀 This looks good to me.
@kevin-bates or @mwakaba2, would either of you be willing to do a quick scan of the code to make sure nothing looks out of the ordinary? After one of you approves, I'll go ahead a merge and release. Thank you!
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.
These changes look good. Not sure if the dismissed codeQL issues in the test code will "come back to life" or not, but let's see. 😄
@@ -42,6 +42,7 @@ install_requires = | |||
prometheus_client | |||
anyio>=3.1.0,<4 | |||
websocket-client | |||
requests-unixsocket |
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.
Is this actually used somewhere?
This is a port of jupyter/notebook#4835. (The changes could not be easily cherry-picked because it requires substantial changes when porting the tests.)
Overview of the changes:
--integration-tests
flag to the pytest CLI options for running the integration tests. Otherwise, these tests are ignored. (They shouldn't really be tested locally).