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

Fix issue #3: HTTPS proxy support through custom agent #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

debrouxl
Copy link

I implemented the outcome of the discussion in issue #3 , and made a quick local test from behind an HTTPS proxy, which accessed the server successfully.

As far as naming is concerned, maybe the property in params should be named "httpsAgent" for consistency, or the "https" / "HTTPS" strings should go away ?

*
* @param {Object} httpsAgent: the custom HTTPS agent
*/
Ovh.prototype.setCustomHTTPSAgent = function (httpsAgent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor is "agent", internal is "httpsAgent", setter is "customHTTPSAgent". Please pick one :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I have picked one in the updated commit :)

@yadutaf
Copy link
Contributor

yadutaf commented Aug 20, 2015

Thanks for your contribution! Quality is a must-have for OVH. Can you provide relevant tests and documentation for this patch? Thanks.

@debrouxl
Copy link
Author

  • do you mean that you'd like to see more documentation comments, a note in the readme, or / in addition to something else ?
  • unit testing would require going through a real HTTP proxy. Would a trivial proxy (pure request forward) created by the test infrastructure for the sole purpose of testing be acceptable ?

@loganmzz
Copy link

Still no support for proxy settings ?

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.

3 participants