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

Support communicating with rstudio via unix socket instead of tcp socket #159

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

jhgoebbert
Copy link
Contributor

@jhgoebbert jhgoebbert commented Jan 1, 2025

This PR only adds the part for unix-socket-support from #151 as @yuvipanda asked in #151 (comment) to better split it.

It enables RStudio connections via sockets in combination with this PR for RStudio rstudio/rstudio#14938 which is part of the official release RStudio v2024.12.0+467.

@yuvipanda
Copy link
Contributor

Excellent @jhgoebbert! The code looks good to me, am spinning up my local test env and will merge once I can verify it.

Thank you so much for your patience!

This allows us to default to using unix sockets by default in the future once
this feature is better tested, and allow people to turn it off if needed.
@yuvipanda
Copy link
Contributor

@jhgoebbert ok, tested this and it looks fine! I have one suggestion that I made as a PR to your repo: jhgoebbert#1.

If that seems ok to you, and you merge that, I'm happy to merge this PR!

@yuvipanda
Copy link
Contributor

This is the Dockerfile I used for testing:

FROM rocker/binder:4

COPY . /tmp/src

USER root

RUN /opt/venv/bin/python3 -m pip install -U -e /tmp/src

USER rstudio

Then I build it with docker buildx build --platform linux/amd64 -t wat .

Then I could run it like docker run -e JUPYTER_RSESSION_PROXY_USE_SOCKET=no -p 8888:8888 -it wat (or other env vars as appropriate)

Explicitly check for 'no' and 'false' in USE_SOCKET env var
@yuvipanda yuvipanda changed the title unix-socket support Support communicating with rstudio via unix socket instead of tcp socket Jan 2, 2025
@yuvipanda yuvipanda merged commit e31e3b1 into jupyterhub:main Jan 2, 2025
1 check passed
@yuvipanda
Copy link
Contributor

Thank you very very much, @jhgoebbert! Would love a PR with the rest of your initial contribution too!

@jhgoebbert
Copy link
Contributor Author

Thank you for merging this, @yuvipanda
I have created another PR with the rest here #160

@yuvipanda
Copy link
Contributor

@jhgoebbert I opened #161 to turn this feature on by default!

@yuvipanda
Copy link
Contributor

Thanks to @sgibson91 there is a new release out now!

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.

3 participants