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

Issue #153: Added socks5 and socks5h support; tests pass #262

Merged
merged 1 commit into from Feb 15, 2017

Conversation

Projects
None yet
2 participants
@RobertFischer
Contributor

RobertFischer commented Feb 12, 2017

I didn't see exactly how to test the usage of a SOCKS5 proxy via the environment variable -- everything that I saw said that it just should work. If you point me in the direction of how to write a test, I'm happy to do so.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 14, 2017

Owner

I don't understand the comment, can you clarify what you mean?

Owner

snoyberg commented Feb 14, 2017

I don't understand the comment, can you clarify what you mean?

@RobertFischer

This comment has been minimized.

Show comment
Hide comment
@RobertFischer

RobertFischer Feb 14, 2017

Contributor

From what I'm understanding, the change that I made will accept "socks5://" and "socks5h://" as the prefix to the HTTPS_PROXY and HTTP_PROXY strings. But I didn't see how to write a test to prove that using those URL prefixes would actually connect us to a SOCKS5 proxy and successfully interact with it.

Contributor

RobertFischer commented Feb 14, 2017

From what I'm understanding, the change that I made will accept "socks5://" and "socks5h://" as the prefix to the HTTPS_PROXY and HTTP_PROXY strings. But I didn't see how to write a test to prove that using those URL prefixes would actually connect us to a SOCKS5 proxy and successfully interact with it.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Feb 15, 2017

Owner

It looks fine to me. Ideally we would have some kind of a docker-compose based test that actually sets up proxy servers, but that doesn't exist right now.

Owner

snoyberg commented Feb 15, 2017

It looks fine to me. Ideally we would have some kind of a docker-compose based test that actually sets up proxy servers, but that doesn't exist right now.

@snoyberg snoyberg merged commit 89d5e6b into snoyberg:master Feb 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RobertFischer RobertFischer deleted the Sapiens-OpenSource:RobertFischer/socks5 branch Feb 20, 2017

snoyberg added a commit that referenced this pull request Feb 20, 2017

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