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 network proxy option #61

Merged
merged 1 commit into from
Aug 29, 2016
Merged

Conversation

v-shan
Copy link

@v-shan v-shan commented Jun 17, 2016

We deploy behind a proxy so we needed the proxy option to be available. Just grabbed from here: http://docs.aws.amazon.com/AWSJavaScriptSDK/guide/node-configuring.html#Configuring_a_Proxy

@v-shan
Copy link
Author

v-shan commented Jun 28, 2016

ping @lukemelia @ghedamat

@ghedamat
Copy link
Contributor

hi @v-shan

sorry for the delay

A small suggestion:

you could use something like
https://github.com/v-shan/ember-cli-deploy-s3/blob/8f62ac24b0f994dbfcc01664f936979777616d9a/lib/s3.js#L44

to pass in the proxyAgent as an option, and use a default

this would also allow you to add a test for your code that mocks it and ensures that proxyAgent(proxy) is called
https://github.com/v-shan/ember-cli-deploy-s3/blob/8f62ac24b0f994dbfcc01664f936979777616d9a/lib/s3.js#L39

similar to what we do here
https://github.com/v-shan/ember-cli-deploy-s3/blob/8f62ac24b0f994dbfcc01664f936979777616d9a/tests/unit/index-nodetest.js#L259-L272

let me know what you think!

@v-shan
Copy link
Author

v-shan commented Jun 28, 2016

Thanks! Will do.

@v-shan v-shan force-pushed the vicki/proxy branch 3 times, most recently from 55d8a1d to 7cc7435 Compare August 29, 2016 19:57
@v-shan
Copy link
Author

v-shan commented Aug 29, 2016

@ghedamat updated! Let me know if I may have misunderstood your suggestion.

Thanks!

@ghedamat
Copy link
Contributor

LGTM! thanks a lot!

@ghedamat ghedamat merged commit 71e84e1 into ember-cli-deploy:master Aug 29, 2016
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