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

Add Proxy support #701

Merged
merged 6 commits into from
Oct 20, 2018
Merged

Add Proxy support #701

merged 6 commits into from
Oct 20, 2018

Conversation

steprescott
Copy link
Contributor

This PR adds the ability to proxy any of the network requests made by danger to be proxied.

It uses the standard HTTP_PROXY & HTTPS_PROXY environment variables and if either of them is set will then proxy those requests.

To ensure the tests pass a server is created so the request can be handled but dose no assertion to ensure the request was proxied. The only addition I have added is adding the Agent to the request and so I have only tested that an Agent is set on the request, not that the request can be successfully proxied as that should be covered by the http-proxy-agent package.

However when running danger pr with either HTTP_PROXY or HTTPS_PROXY set I can see that the requests are being proxied via Charles.
screenshot 2018-10-18 at 12 36 10

More details can be found here.

@DangerCI
Copy link

DangerCI commented Oct 18, 2018

New dependencies added: @types/http-proxy-agent.

@types/http-proxy-agent

Author: Unknown

Description: TypeScript definitions for http-proxy-agent

Homepage: http://npmjs.com/package/@types/http-proxy-agent

Created11 months ago
Last Updated2 days ago
LicenseMIT
Maintainers1
Releases2
Direct Dependencies@types/node
README

Installation

npm install --save @types/http-proxy-agent

Summary

This package contains type definitions for http-proxy-agent (https://github.com/TooTallNate/node-http-proxy-agent).

Details

Files were exported from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/http-proxy-agent

Additional Details

  • Last updated: Wed, 17 Oct 2018 16:01:04 GMT
  • Dependencies: node
  • Global values: none

Credits

These definitions were written by mrmlnc https://github.com/mrmlnc, steprescott https://github.com/steprescott.

Generated by 🚫 dangerJS

package.json Outdated
@@ -129,7 +130,6 @@
"commander": "^2.18.0",
"debug": "^4.0.1",
"get-stdin": "^6.0.0",
"https-proxy-agent": "^2.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

I think you just removed this dependency by accident

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I thought by including @types/http-proxy-agent this was no longer needed. It builds & the tests pass so I'm confused. Is this needed? Happy to revert this line.

Copy link
Member

Choose a reason for hiding this comment

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

It's needed - the @types go in dev dependencies and will only occur in this repo. Not in a consumer of danger

@orta
Copy link
Member

orta commented Oct 19, 2018

Other than that - looks good to me

@steprescott
Copy link
Contributor Author

Thanks for the review. I've acted on your comments. Any more issue let me know. Once this is in I will look at adding the same change to Peril.

@orta
Copy link
Member

orta commented Oct 20, 2018

👍

@orta orta merged commit 6ca39c3 into danger:master Oct 20, 2018
@peril-staging
Copy link
Contributor

peril-staging bot commented Oct 20, 2018

Thanks for the PR @steprescott.

The Danger org conform to the Moya Community Continuity Guidelines, which means
that we want to offer any contributor the ability to control their destiny.

So, we've sent you an org invite - thanks steprescott 🎉

Generated by 🚫 dangerJS

@peril-staging
Copy link
Contributor

peril-staging bot commented Oct 20, 2018

Thanks for the PR @steprescott.

This PR has been shipped in v4.4.10 - CHANGELOG.

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