added netrc support #177
added netrc support #177
Conversation
httpx/config.py
Outdated
| """ | ||
|
|
||
| def __init__(self, *, cert: CertTypes = None, verify: VerifyTypes = True): | ||
| def __init__( |
There was a problem hiding this comment.
Let's revert usages of trust_env for SSLConfig. The default behavior in the future should be use system trust stores unless we're specifically told which cert bundle to use.
Although this change reminded me to create another issue for SSL_CERT_... environment variables.
There was a problem hiding this comment.
You're right new approach for environment variables would be nice.
I reverted the last commit.
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
So the whole trust_env parameter is kind of a can of worms. Ian, Cory, and @nateprewitt have all dealt with it and there are large issue threads i can dig up about all the issues.
The biggest issue is basically we're going to be embedded into applications/tools that don't expose a way to control how we configure ourselves but users still need to configure us for proxies, authentication, certificates, etc. I think the best way to achieve that is still environment variables but then you get into priorities like "both REQUESTS_CA_CERT and SSL_CERT_FILE ste defined and are different, what do we do?"
We need answers to these questions for sure, maybe a variable that can control order?
lovelydinosaur
left a comment
lovelydinosaur
left a comment
There was a problem hiding this comment.
Looks really good yeah! 👍
Feel questions and comments for ya.
httpx/utils.py
Outdated
| return None | ||
|
|
||
|
|
||
| def get_netrc_login(url: str) -> typing.Union[typing.Tuple[str, str, str], None]: |
There was a problem hiding this comment.
Signature should either be host or authority here, right?
It's not clear to me from a glance at the netrc docs, which of those two is actually the correct thing to use?
Also we should prefer typing.Optional[typing.Tuple[str, str, str]] as the return annotation.
There was a problem hiding this comment.
machine in the config. I changed it as host I can change again if necessary
httpx/utils.py
Outdated
|
|
||
| def get_netrc_login(url: str) -> typing.Union[typing.Tuple[str, str, str], None]: | ||
| try: | ||
| _netrc = netrc.netrc(os.environ.get("DEFAULT_NETRC_PATH")) # type: ignore |
There was a problem hiding this comment.
I'd probably prefer a variable name that's not underscore prefixed here. netrc_info maybe?
There was a problem hiding this comment.
Is DEFAULT_NETRC_PATH a standardized thing? (Or used by requests or something?)
There was a problem hiding this comment.
I made it up to explain the initial path.
There was a problem hiding this comment.
I'm pretty sure NETRC is the env variable to specify the path
tests/.netrc
Outdated
| @@ -0,0 +1,3 @@ | |||
| machine test_entrc.httpx.com | |||
There was a problem hiding this comment.
Let's use the example.org domain, since it's there for this sort of thing.
There was a problem hiding this comment.
If appropriate, I used netrcexample.org. Because some tests are affected by example.org.
tests/.netrc
Outdated
| @@ -0,0 +1,3 @@ | |||
| machine test_entrc.httpx.com | |||
| login tomchristie | |||
| password password123 No newline at end of file | |||
There was a problem hiding this comment.
I'd probably marginally prefer that we use a fake style username here example-username and example-password or something. Just feels marginally cleaner.
|
Thanks, I applied your comments. |
tests/client/test_auth.py
Outdated
| assert response.json() == {"auth": "Token 123"} | ||
|
|
||
|
|
||
| def test_entrc_auth(): |
There was a problem hiding this comment.
Typo: entrc -> netrc
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
I've got no issue with this current implementation.
|
Great stuff, thanks! |
4 participants
No description provided.