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

Move proxy_request_options to configuration. #113

Closed
wants to merge 3 commits into from

Conversation

ryanlovett
Copy link
Collaborator

To address #112.

@ryanlovett
Copy link
Collaborator Author

Example app that requires a proxy request option:
https://github.com/ryanlovett/jupyter-syncthing

@yuvipanda
Copy link
Contributor

This is awesome, and we should definitely support configuring timeouts. However, I think we should explicitly support them, since otherwise we will be tied down to Tornado's HTTP Client implementation. I'd love to move to aiohttp at some point.

@costa
Copy link

costa commented Sep 23, 2019

Hello, thank you for your effort.

I need to configure jupyterhub proxy timeouts (without monkey-patching the code preferably).

I'll just wait for this to be merged, right?

It cannot take long, definitely under half a year, right?

@manics
Copy link
Member

manics commented Sep 23, 2019

@costa Are you able to help test this?

@costa
Copy link

costa commented Sep 23, 2019

@manics Let's see, I'm using the JupyterHub helm chart with a custom-built singleuser image which is built with installing jupyter-server-proxy through pip; I guess I can install a custom version of that to produce a test image to use with a test JupyterHub deployment — if it is not too much effort to do so.

@costa
Copy link

costa commented Sep 26, 2019

Guys (no assumption here ;)), this PR has conflicts, I'm probably willing to test this if I have a chance to change the proxy request timeout without having to monkey-patch the code myself — and anyone can (fix the conflicts and) confirm it's worth the effort.

@consideRatio consideRatio changed the title [WIP] Move proxy_request_options to configuration. Move proxy_request_options to configuration. Nov 1, 2022
@consideRatio consideRatio marked this pull request as draft November 1, 2022 20:53
@yuvipanda
Copy link
Contributor

It cannot take long, definitely under half a year, right?

:D

I think unfortunately this PR didn't make it.

While we do have a process ready timeout (

) we don't have a request timeout.

I think we should implement request timeout as its own option, rather than allowing arbitrary options to be passed through. That exposes internal detail of the project (which http client it uses) outside, making it impossible to change.

With that, I'm going to close this.

@yuvipanda yuvipanda closed this Jun 11, 2024
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.

None yet

4 participants