-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Implementing HTTP and HTTPS client proxy support for non-Windows plat… #41
Conversation
…forms. Fixing whitespace
Hi @deeringc, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
@deeringc, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Thank you for the pull request. This will be merged as soon as the project owners get back from vacation. |
|
||
http_client_config m_client_config; | ||
http_client_config m_client_config; |
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.
exposing non-const
access to this data member could be dangerous. The one new use in http_client_asio.cpp can call the member function client_config() for this.
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.
OK, I'll refactor
Right now we do not have any test cases that exercise the HTTPS proxy code paths. Would it be possible to add a test case? Can we use something like httpbin.org? I think it has HTTPS too, though I am not 100% sure. Another option is to add a manual testcase (with the tag "Ignore", "Manual") and anyone running the testcase can modify the credentials locally. I am OK if we cannot add a testcase too. |
Yeah, I was thinking about adding another test case that covers https. I wasn't sure that your internal Microsoft proxy supported this (we don't have access from outside of your network) and didnt want to add a potentially failing test that I couldn't verify myself. I'll do as you suggest and add a manual test. If we come up with some other idea, we can just enable it later. |
These changes have been merged to the development branch. Release 2.8.0 (under planning) should have them in the master branch. |
As discussed here: #9