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 socks proxy support #253

Merged
merged 2 commits into from
Jan 28, 2021
Merged

Conversation

Zyrix
Copy link
Contributor

@Zyrix Zyrix commented Jan 15, 2021

This PR adds support for SOCKS proxies. It uses the "proxy" parameter for SOCKS proxies and distinguishes SOCKS proxies from other proxies by the occurence of "socks" in the proxy url.
urllib3 is used for SOCKS proxies which uses pysocks for its implementation. See https://urllib3.readthedocs.io/en/latest/reference/contrib/socks.html

@coveralls
Copy link

coveralls commented Jan 15, 2021

Coverage Status

Coverage increased (+0.7%) to 69.11% when pulling 66446a0 on Zyrix:add-socks-proxy-support into d00221b on Chaffelson:main.

@Chaffelson
Copy link
Owner

Hi @Zyrix thanks for this contribution!
It looks relatively straight forward, which is great, can I ask you to consider two things please.
Firstly, is there some tests that can be easily integrated for this? Perhaps a socks proxy on a docker container could be added to our docker compose test environment, and appropriate tests run against that.
Secondly, the client is generated from mustache templates, including the rest implementation. It would be good to include these changes in the template so that future clients do not drop them unexpectedly, you can find the template here:
https://github.com/Chaffelson/nipyapi/blob/main/resources/client_gen/swagger_templates/rest.mustache

Both of these I am happy to help with if you like, please let me know your thoughts.

@Zyrix
Copy link
Contributor Author

Zyrix commented Jan 25, 2021

Thank you for your comments! I added my changes to the moustache template. As for the docker proxy test I'm not sure how to implement this and would be glad if you could help me with that.

@Chaffelson Chaffelson merged commit 5480af8 into Chaffelson:main Jan 28, 2021
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