-
Notifications
You must be signed in to change notification settings - Fork 19
Implement configuration-based SOCKS proxy support #1428
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
Conversation
Generate changelog in
|
49a45bf to
4671c3d
Compare
| log.debug("No SOCKS proxy found"); | ||
| } | ||
| return address; | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little scary (as in might hide errors). Is this just to avoid breaking existing stuff in prod in case there is a problem with the socks detection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, this should log at ERROR rather than DEBUG 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, I added the catch to avoid adding risk to the production case.
|
👍 |
...ogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java
Outdated
Show resolved
Hide resolved
…/hc5/ApacheHttpClientChannels.java
|
Released 3.30.0 |
Depends on:
palantir/conjure-java-runtime-api#699
palantir/conjure-java-runtime#2126
After this PR
==COMMIT_MSG==
Implement configuration-based SOCKS proxy support
==COMMIT_MSG==
Possible downsides?
More proxies, socks is particularly difficult to test