Add support for sending events through proxy #397
Conversation
lib/transports.js
Outdated
| proxy: { | ||
| host: options.proxyHost, | ||
| port: options.proxyPort, | ||
| proxyAuth: null // TODO: Add ability to specify creds/auth |
There was a problem hiding this comment.
will take care of this (for HTTPProxyTransport also)
|
@kamilogorek Can you please review and give me your thoughts/comments? (I know |
|
Still need to write tests |
ndmanvar
force-pushed
the
add_proxy_support
branch
3 times, most recently
from
33203dc
to
954ee7a
Compare
For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.
ndmanvar
force-pushed
the
add_proxy_support
branch
from
954ee7a
to
6cce30b
Compare
ndmanvar
force-pushed
the
add_proxy_support
branch
from
21febae
to
9a8a4c2
Compare
|
@kamilogorek can you please review when you get a chance? (: |
|
@ndmanvar will do! I just wasn't sure if you're done with the tests :) |
| httpsProxyTransport.options.agent.proxyOptions.should.exist; | ||
|
|
||
| var _cachedAgent = httpsProxyTransport.agent; | ||
| var requests = {}; |
There was a problem hiding this comment.
You don't seem to use this object anywhere in tests
| @@ -15,6 +15,92 @@ describe('transports', function() { | |||
| https.agent.maxSockets.should.equal(100); | |||
| }); | |||
|
|
|||
| it('should set HTTPS proxy transport when proxy config is specified and request is sent', function(done) { | |||
| var option = { | |||
| this.transport = http; | ||
| this.options = options || {}; | ||
| this.agent = httpAgent; | ||
| this.options.host = options.proxyHost; |
There was a problem hiding this comment.
How about just using host port etc. instead? If we pass options object to this constructor, it's by definition mentioned to be used by proxy, therefore using a prefix proxyHost seems redundant.
There was a problem hiding this comment.
Or it may be useful to distinguish http from httpProxy options. It's up to you really.
| null, | ||
| null, | ||
| function() { | ||
| httpsProxyTransport.options.agent.proxyOptions.headers.host.should.exist; |
There was a problem hiding this comment.
We could also check that it has been created correctly and assert on foo:123
|
@MaxBittker could you also take a peak at this code as well? |
| 'api/' + | ||
| client.dsn.project_id + | ||
| '/store/'; | ||
| delete options.hostname; // only 'host' should be set when using proxy |
There was a problem hiding this comment.
slightly better comment here for what will happen if you send hostname or a link to a resource that justifies why?
| delete options.hostname; // only 'host' should be set when using proxy | ||
|
|
||
| if (this.options.proxyAuth) { | ||
| // might be able to use httpOverHttp agent |
There was a problem hiding this comment.
could this be commented to be more explanatory? not sure what these two branches mean
|
Any updates on this? Would lodging a support request as a paying customer expedite this in anyway? |
For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.