Skip to content

support $no_proxy#192

Merged
damccorm merged 2 commits intomicrosoft:masterfrom
gforceg:master
Oct 8, 2018
Merged

support $no_proxy#192
damccorm merged 2 commits intomicrosoft:masterfrom
gforceg:master

Conversation

@gforceg
Copy link
Copy Markdown
Contributor

@gforceg gforceg commented Jun 29, 2018

if the host of the TFS instance is listed in the $no_proxy variable do
not set options.proxy

Feature request #183

@msftclas
Copy link
Copy Markdown

msftclas commented Jun 29, 2018

CLA assistant check
All CLA requirements met.

@nschonni
Copy link
Copy Markdown

nschonni commented Jul 9, 2018

@stephenmichaelf could you take a look at this ❤️ ❤️ ❤️
This causes issues for on-prem people sitting behind proxies when they try to call their instances and hit 407 issues (along with traffic hitting the proxy that shouldn't)

EX: microsoft/tfs-cli#214 and microsoft/tfs-cli#78

@stephenmichaelf
Copy link
Copy Markdown
Member

Will take a look today! Thanks.

Comment thread api/WebApi.ts Outdated
};

this.options.proxy = proxyFromEnv;
if (!process.env.no_proxy || process.env.no_proxy.toLowerCase().split(',').indexOf(url.parse(this.serverUrl).host) !== -1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@gforceg @nschonni Has this been tested to make sure it works as desired? I think this is backwards. It should be === instead of !==, right? We are trying to enter this block of code only if the server host is not in no_proxy.

        var no_proxy = 'www.google.com';
        var serverUrlHost = 'www.google.com';

        if (!no_proxy || no_proxy.toLowerCase().split(',').indexOf(serverUrlHost) === -1) {
            console.log('allowed setting proxy');
        }
        else {
            console.log('skipped setting proxy');
        }

Also probably want toLowerCase on serverUrl if url.parse doesn't do that for you.

I'm not opposed to extracting the logic to a function.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Think you're right. Looking at https://github.com/request/request/blob/b12a6245d9acdb1e13c6486d427801e123fdafae/lib/getProxyFromURI.js there may be some additional logic needed too

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the link Nick, appreciate it. IMO all the more reason to extract this to a method.

@gforceg Could you make the suggested changes? Thanks! Appreciate the contribution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stephenmichaelf, will do, thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@gforceg Any updates on this work?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@gforceg any updates? Are you still planning on submitting this PR?

Copy link
Copy Markdown
Contributor Author

@gforceg gforceg Sep 23, 2018

Choose a reason for hiding this comment

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

Sorry for the delay on this.
First I was on vacation then I was doing other work.

I wrapped the functionality into a function and then rebased.

Can you provide guidance on where I can stick a unit test?
samples/common.ts mentions proxy but I'm still trying to understand the how the samples / tests work in this project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

haha I just noticed the tests directory.
Added a unit test. Still needs to be e2e tested.
@nschonni, can you give this a try?

@damccorm
Copy link
Copy Markdown

This looks good to me, thanks for the contribution/follow up! @stephenmichaelf are you good with merging?

@damccorm
Copy link
Copy Markdown

damccorm commented Oct 8, 2018

Just double checked, this looks good, I'm going to merge. Thanks for the contribution!

@damccorm damccorm merged commit bb15a2d into microsoft:master Oct 8, 2018
@damccorm damccorm mentioned this pull request Oct 8, 2018
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.

5 participants