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

[Notebook port 4835] Add UNIX socket support to notebook server #525

Merged
merged 2 commits into from
May 24, 2021

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented May 20, 2021

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:

  • enables connection to a jupyter server via UNIX sockets.
  • adds integration tests for launching, listing, and shutting-down UNIX socket servers.
  • runs the integration tests in a new, separate Github workflow.
  • adds a --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).
  • improves, adds, and tests the handling of some public properties related to the ServerApp's URLs.
  • adds some utility functions for making sync/async HTTP or UNIX socket requests to a running Jupyter server.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2021

Codecov Report

Merging #525 (9c25147) into master (5f76fda) will decrease coverage by 0.66%.
The diff coverage is 53.69%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...r/tests/unix_sockets/test_serverapp_integration.py 22.72% <22.72%> (ø)
jupyter_server/serverapp.py 64.88% <43.79%> (-1.89%) ⬇️
jupyter_server/utils.py 63.15% <66.00%> (+1.02%) ⬆️
jupyter_server/tests/conftest.py 78.57% <76.92%> (-21.43%) ⬇️
jupyter_server/base/handlers.py 63.91% <77.77%> (+0.22%) ⬆️
jupyter_server/tests/unix_sockets/conftest.py 94.44% <94.44%> (ø)
jupyter_server/tests/unix_sockets/test_api.py 97.29% <97.29%> (ø)
jupyter_server/__init__.py 64.28% <100.00%> (+2.74%) ⬆️
jupyter_server/tests/test_serverapp.py 99.09% <100.00%> (+0.11%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f76fda...9c25147. Read the comment docs.

@kevin-bates
Copy link
Member

I went ahead and dismissed the two remaining codeQL issues since they were only in test code (side effect stuff).

Copy link
Member

@Zsailer Zsailer left a 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!

Copy link
Member

@kevin-bates kevin-bates left a 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. 😄

@blink1073 blink1073 merged commit 40c477b into jupyter-server:master May 24, 2021
@@ -42,6 +42,7 @@ install_requires =
prometheus_client
anyio>=3.1.0,<4
websocket-client
requests-unixsocket
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants