@kamilogorek Can you please review and give me your thoughts/comments? (I know proxyAuth is still incomplete/TODO but wanted to get your thoughts/review on the rest of it)

@ndmanvar

Copy link

Author

ndmanvar commented Nov 17, 2017

Still need to write tests

@ndmanvar ndmanvar force-pushed the add_proxy_support branch 3 times, most recently from 33203dc to 954ee7a Compare

November 17, 2017 09:32

@ndmanvar ndmanvar requested a review from MaxBittker

November 17, 2017 09:33

@ndmanvar


 Add support for sending events through proxy

6cce30b

For both HTTP and HTTPS endpoints. Uses tunnel-agent for https.

@ndmanvar ndmanvar force-pushed the add_proxy_support branch from 954ee7a to 6cce30b Compare

November 17, 2017 17:47

ndmanvar added 2 commits

January 4, 2018 16:35

@ndmanvar


 Add proxyAuth (ability to specify user/password for proxy)

de6da9e

@ndmanvar


 remove require buffer

9a8a4c2

@ndmanvar ndmanvar force-pushed the add_proxy_support branch from 21febae to 9a8a4c2 Compare

January 5, 2018 03:13

ndmanvar added 2 commits

January 5, 2018 01:33

@ndmanvar


 Add initial test

8abbf65

@ndmanvar


 Make change to this.options and fix unit tests

1bc3de5

@ndmanvar

Copy link

Author

ndmanvar commented Jan 11, 2018

@kamilogorek can you please review when you get a chance? (:

@kamilogorek

Copy link

Contributor

kamilogorek commented Jan 12, 2018

@ndmanvar will do! I just wasn't sure if you're done with the tests :)

kamilogorek

kamilogorek suggested changes

Jan 16, 2018

View reviewed changes

Copy link

Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Would you mind adding some docs to describe how to use proxies as well? :)

test/raven.transports.js

httpsProxyTransport.options.agent.proxyOptions.should.exist;

var _cachedAgent = httpsProxyTransport.agent;

var requests = {};

Copy link

Contributor

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

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

You don't seem to use this object anywhere in tests

test/raven.transports.js

});

it('should set HTTPS proxy transport when proxy config is specified and request is sent', function(done) {

var option = {

Copy link

Contributor

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

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

s/option/options/g

lib/transports.js

this.transport = http;

this.options = options || {};

this.agent = httpAgent;

this.options.host = options.proxyHost;

Copy link

Contributor

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link

Contributor

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

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

Or it may be useful to distinguish http from httpProxy options. It's up to you really.

test/raven.transports.js

null,

function() {

httpsProxyTransport.options.agent.proxyOptions.headers.host.should.exist;

Copy link

Contributor

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

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

We could also check that it has been created correctly and assert on foo:123

@kamilogorek

Copy link

Contributor

kamilogorek commented Jan 16, 2018

@MaxBittker could you also take a peak at this code as well?

MaxBittker

MaxBittker approved these changes

Feb 21, 2018

View reviewed changes

Copy link

Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

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

hard to review some of the domain-specific logic, but if people are using some version of this successfully already that's a good signal! agree with kamil's comments, but i think this is looking good overall.

lib/transports.js

'api/' +

client.dsn.project_id +

'/store/';

delete options.hostname; // only 'host' should be set when using proxy

Copy link

Contributor

@MaxBittker MaxBittker Feb 21, 2018

Choose a reason for hiding this comment

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

slightly better comment here for what will happen if you send hostname or a link to a resource that justifies why?

lib/transports.js

delete options.hostname; // only 'host' should be set when using proxy

if (this.options.proxyAuth) {

// might be able to use httpOverHttp agent

Copy link

Contributor

@MaxBittker MaxBittker Feb 21, 2018

Choose a reason for hiding this comment

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

could this be commented to be more explanatory? not sure what these two branches mean

@ghost

Copy link

ghost commented May 28, 2018

Any updates on this? Would lodging a support request as a paying customer expedite this in anyway?

@ndmanvar


 Merge branch 'master' into add_proxy_support

33fabb3

Labels

Status: In Progress Type: Feature

3 participants

@ndmanvar @kamilogorek @MaxBittker

Uh oh!

Comments

Conversation

@ndmanvar ndmanvar commented Nov 17, 2017

Uh oh!

@ndmanvar ndmanvar Nov 17, 2017

Choose a reason for hiding this comment

Uh oh!

ndmanvar commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndmanvar commented Nov 17, 2017

Uh oh!

ndmanvar commented Jan 11, 2018

Uh oh!

kamilogorek commented Jan 12, 2018

Uh oh!

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

Uh oh!

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

@kamilogorek kamilogorek Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

kamilogorek commented Jan 16, 2018

Uh oh!

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

Uh oh!

@MaxBittker MaxBittker Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

@MaxBittker MaxBittker Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented May 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ndmanvar commented Nov 17, 2017 •

edited

Loading