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

Mention HTTP_PROXY if download error occurs; fall back to NPM's proxy… #4705

Merged
merged 5 commits into from
Jul 15, 2019

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Jul 12, 2019

@flotwig flotwig changed the title [WIP] Mention HTTP_PROXY if download error occurs; fall back to NPM's proxy… Mention HTTP_PROXY if download error occurs; fall back to NPM's proxy… Jul 12, 2019
@flotwig flotwig requested a review from a team July 12, 2019 17:56
cli/lib/tasks/download.js Outdated Show resolved Hide resolved
@brian-mann
Copy link
Member

This will download the binary but still fail when running actual Cypress tests because we don't check those same npm config env vars right?

@brian-mann
Copy link
Member

Should we update the docs to make mention of us respecting the npm config env vars?

@flotwig
Copy link
Contributor Author

flotwig commented Jul 12, 2019

This will download the binary but still fail when running actual Cypress tests because we don't check those same npm config env vars right?

~~ Correct, I think it makes sense to respect the npm config vars here because it's part of the npm install when they download the package. ~~

I figure that Cypress might not be launched through NPM, so npm_config might not be available, so we should only accept HTTP_PROXY in the server for it to behave uniformly. Imagine, users reporting issues like "Cypress can access the Internet when I launch it through NPM but not when I launch it with cypress open". Other command line tools have standardized on HTTP_PROXY too, so they should be familiar with this kind of setup. Not sure, do you think it'd be best to allow npm_config_proxy as a fallback in the server as well?

Never mind, let's just use it and assume that people aren't switching back and forth like that. I'll add add'l logging so that we can debug this issue if it does crop up.

Should we update the docs to make mention of us respecting the npm config env vars?

The npm install will fail before reaching Cypress if they don't have their proxy configured properly, so I don't think it's necessary to mention in our docs. They'll have to configure npm config proxy anyways before Cypress comes into the picture.

@flotwig flotwig requested a review from bahmutov July 12, 2019 18:50
bahmutov
bahmutov previously approved these changes Jul 12, 2019
Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

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

nice

@brian-mann brian-mann self-requested a review July 15, 2019 16:50
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

  • respect npm_config env vars if set when in cypress run or cypress open
  • add nice debug logs for the proxy to indicate where/what set the values

Copy link
Contributor

@bahmutov bahmutov left a comment

Choose a reason for hiding this comment

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

looks good. I am wondering, should we have an example recipe that shows and tests the proxy env variable

@brian-mann
Copy link
Member

@bahmutov I don't think so - proxies aren't really something that users desire to have setup - they are constraints that exist in their work environment that they have to conform to. It's not like "a feature" or anything.

As long as we provide good documentation / error messaging, etc that help guide and direct those who are affected I think we're fine. Have we had anyone open issues or ask support questions in regards to how to utilize their proxies? I'm not aware of any TBH.

@brian-mann brian-mann self-requested a review July 15, 2019 18:13
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

@flotwig this PR is actually doing two things - adding a message but also tapping into the npm config for proxy support. Can you open a separate issue for that (since it's a discrete thing that needs to be mentioned in the changelog and also noted in the docs).

@flotwig
Copy link
Contributor Author

flotwig commented Jul 15, 2019

Can you open a separate issue for that (since it's a discrete thing that needs to be mentioned in the changelog and also noted in the docs).

Done, #4719

@flotwig flotwig requested a review from brian-mann July 15, 2019 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants