Skip to content

feat: warn if apify proxy is used in proxyUrls#1173

Merged
B4nan merged 2 commits intomasterfrom
warn-proxy
Sep 10, 2021
Merged

feat: warn if apify proxy is used in proxyUrls#1173
B4nan merged 2 commits intomasterfrom
warn-proxy

Conversation

@szmarczak
Copy link
Copy Markdown
Contributor

Fixes #1119

Comment thread src/proxy_configuration.js Outdated
this.isManInTheMiddle = false;

if (proxyUrls && proxyUrls.some((url) => url.includes('apify.com'))) {
this.log.warning('Some Apify proxy features may work incorrectly. Please consider setting up Apify properties instead of `proxyUrls`.');
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.

by "Apify properties" you mean env vars? maybe we should suggest login via CLI?

Copy link
Copy Markdown
Contributor Author

@szmarczak szmarczak Sep 10, 2021

Choose a reason for hiding this comment

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

Anything. Can be env vars, cli, groups and password in ProxyConfiguration etc.

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.

maybe we could add a link to the docs to make it more clear

https://sdk.apify.com/docs/guides/proxy-management#apify-proxy-configuration

or maybe better

https://bit.ly/3l99Ijj

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.

Suggested change
this.log.warning('Some Apify proxy features may work incorrectly. Please consider setting up Apify properties instead of `proxyUrls`.');
this.log.warning('Some Apify proxy features may work incorrectly. See https://sdk.apify.com/docs/guides/proxy-management#apify-proxy-configuration');

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.

?

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.

i'd keep the other sentence too

@szmarczak szmarczak requested a review from B4nan September 10, 2021 14:19
@B4nan B4nan merged commit 682f8b7 into master Sep 10, 2021
@B4nan B4nan deleted the warn-proxy branch September 10, 2021 14:26
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.

Accept Apify proxies in proxyUrls

2 participants