Skip to content
This repository was archived by the owner on Nov 20, 2025. It is now read-only.

fix: support HTTPS proxies#405

Merged
JustinBeckwith merged 2 commits intogoogleapis:masterfrom
JustinBeckwith:proxy
Aug 29, 2018
Merged

fix: support HTTPS proxies#405
JustinBeckwith merged 2 commits intogoogleapis:masterfrom
JustinBeckwith:proxy

Conversation

@JustinBeckwith
Copy link
Copy Markdown
Contributor

@JustinBeckwith JustinBeckwith commented Jul 4, 2018 •
edited
Loading

Uses https-proxy-agent to provide HTTPS proxy support, making up for the fact that axios doesn't support it natively yet.

I don't have an HTTPS proxy sitting around where I can test this, so I am really throwing stuff at the wall here.

Also of note - there is no reason I can possibly come up with why we would ever support an HTTP proxy. Every endpoint we're going to hit for a Google service will be HTTPS, so I want to explicitly remove that from the documentation.

Fixes #352

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 4, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 4, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@3ba8c45). Click here to learn what that means.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #405   +/-   ##
=========================================
  Coverage          ?   94.79%           
=========================================
  Files             ?       15           
  Lines             ?      961           
  Branches          ?      219           
=========================================
  Hits              ?      911           
  Misses            ?       50           
  Partials          ?        0
Impacted Files Coverage Δ
src/transporters.ts 92.59% <60%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ba8c45...d998dbe. Read the comment docs.

@JustinBeckwith JustinBeckwith mentioned this pull request Jul 4, 2018
Closed
@JustinBeckwith JustinBeckwith requested a review from ofrobots July 4, 2018 21:25
ofrobots
ofrobots approved these changes Jul 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Let's wait for positive confirmation from users before landing this.

ofrobots
ofrobots reviewed Jul 6, 2018
View reviewed changes
src/transporters.ts Outdated

This comment was marked as spam.

Sign in to view

yonilerner
yonilerner reviewed Jul 6, 2018
View reviewed changes
src/transporters.ts Outdated

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@JustinBeckwith JustinBeckwith added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 12, 2018
@fhinkel
Copy link
Copy Markdown
Contributor

fhinkel commented Aug 28, 2018

@JustinBeckwith , why did we close this without merging? Did we realize this is not going to work? Looking through the comments this looks promising to me.

@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Aug 29, 2018

My thought at the time was that this wouldn't be effective since it only applies to the auth library, not every library we have. That having been said, it's worth reconsidering!

@fhinkel
Copy link
Copy Markdown
Contributor

fhinkel commented Aug 29, 2018

Which other libraries use axios? It's only a handful, right?

@JustinBeckwith JustinBeckwith removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. in progress labels Aug 29, 2018
@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Aug 29, 2018

Fair point :)

@JustinBeckwith JustinBeckwith merged commit 5377684 into googleapis:master Aug 29, 2018
@fhinkel fhinkel mentioned this pull request Aug 29, 2018
Closed
@danielbankhead danielbankhead mentioned this pull request Nov 15, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@yonilerner yonilerner yonilerner left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@JustinBeckwith @codecov-io @fhinkel @ofrobots @yonilerner @googlebot