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

Extended conan.conf syntax to permit host-specific proxies #4923

Merged
merged 5 commits into from
May 23, 2019

Conversation

dawidcha
Copy link
Contributor

@dawidcha dawidcha commented Apr 6, 2019

Changelog: (Feature): Extend syntax to permit host-specific proxies (backward compatible)
Docs: conan-io/docs#1241

@CLAassistant
Copy link

CLAassistant commented Apr 6, 2019

CLA assistant check
All committers have signed the CLA.

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 6, 2019

Just noticed I'd left 'import sys' in used for debugging - it can be removed.

@memsharded
Copy link
Member

This PR looks useful and nice that it seems backwards compatible, thanks for contributing it.

It also seems that it would be necessary to bump the minimum requirement of requests to 2.8.1, as the one that is currently declared in requirements.txt (2.7.0), won't be enough: http://docs.python-requests.org/en/v2.8.1/user/advanced/

@lasote do you recall some reason why we were requesting 2.7.0? Could it be upgraded?

@lasote
Copy link
Contributor

lasote commented Apr 8, 2019

I cannot recall, probably we are good upgrading requests.

conf = """
[proxies]
https=http://conan.url
only.for.this.conan.url = http://special.url
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would need a test with more than 1 host line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as requested

@lasote lasote added this to the 1.15 milestone Apr 8, 2019
@jgsogo
Copy link
Contributor

jgsogo commented Apr 8, 2019

Hi! Thanks for the contribution

One suggestion to consider: IMO, it is easier to understand (and to parse) a plain list of schema[://hostname] = proxy that will be translated directly into a dictionary:

[proxies]
https = http://default-proxy.acme.com
https://conan-repo.cloud.acme.com = http://cloud-proxy.acme.com
http = http://other.proxy

I assume that requests module will handle each url from most specific to most generic one, so the order in the dictionary is not important (no need for an OrderedDict).

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 8, 2019

I'm away from my Dev rig at present, but fwiw, I did run a manual integration test - ran:

conan search Poco* -r conan-center

  • Withdefault conan.conf
    (success)
  • With
https=http://1.2.3.4

(can't find proxy)

  • With
https=
       conan.bintray.com=http://1.2.3.4

(can't find proxy)

  • With
https=
       google.com=http://1.2.3.4

(Success)

Not totally conclusive, but it suggests requests is working off the develop branch. Maybe it's an undocumented feature in 2.7.0?

I'll improve the test as suggested this (UK) evening

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 8, 2019

Hi! Thanks for the contribution

One suggestion to consider: IMO, it is easier to understand (and to parse) a plain list of schema[://hostname] = proxy that will be translated directly into a dictionary:

[proxies]
https = http://default-proxy.acme.com
https://conan-repo.cloud.acme.com = http://cloud-proxy.acme.com
http = http://other.proxy

This doesn't work as ConfigParser treats the : in the URL as a name/Val separator, by default. That's configurable, but not in a way that retains 100% backward compatibility

I assume that requests module will handle each url from most specific to most generic one, so the order in the dictionary is not important (no need for an OrderedDict).

Yes, I kinda assumed that also. The docs aren't clear. I can investigate.

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 8, 2019

Note: the proxy search code does indeed search for host-constrained proxies before general proxies

From 2.8.1
https://github.com/kennethreitz/requests/blob/v2.8.1/requests/utils.py : select_proxy(...)

Looks like my install had pulled in 2.21.0 which also worked.

@jgsogo
Copy link
Contributor

jgsogo commented Apr 9, 2019

btw, proxies are documented here in the reference: https://docs.conan.io/en/latest/reference/config_files/conan.conf.html#proxies

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 9, 2019

btw, proxies are documented here in the reference: https://docs.conan.io/en/latest/reference/config_files/conan.conf.html#proxies

Aha, thanks. I'll update and submit a docs pull request

@dawidcha
Copy link
Contributor Author

dawidcha commented Apr 9, 2019

Pull request for docs here
conan-io/docs#1241

@memsharded
Copy link
Member

Hi @dawidcha

I have proposed in dawidcha#1 some minor improvements, but also bumping the min version of requests to 2.8.1, I think this PR should do it.

Thanks again for your contribution, great job!

@lasote
Copy link
Contributor

lasote commented Apr 16, 2019

@dawidcha Could you take a look to the @memsharded PR? Thanks!

@lasote
Copy link
Contributor

lasote commented Apr 26, 2019

ping @dawidcha

@lasote lasote removed this from the 1.15 milestone Apr 30, 2019
@lasote lasote added the inactive No comments for a long time label Apr 30, 2019
@lasote lasote added this to the 1.16 milestone May 3, 2019
@lasote
Copy link
Contributor

lasote commented May 21, 2019

pong @dawidcha

@lasote lasote removed this from the 1.16 milestone May 21, 2019
@memsharded memsharded removed the inactive No comments for a long time label May 23, 2019
@memsharded memsharded added this to the 1.16 milestone May 23, 2019
@memsharded memsharded merged commit 71cc3a3 into conan-io:develop May 23, 2019
@memsharded
Copy link
Member

Didn't want to have this waiting for so long, as it was good.
Merged with develop, solved conflicts, merged my small refactors and merged.

Will be in Conan 1.16, thanks @dawidcha for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[proxies] section in conan.conf does not permit proxies for specific targets
5 participants