SQL-3010: fallback to default dns resolver on windows #356
Conversation
nicholascioli
left a comment
nicholascioli
left a comment
There was a problem hiding this comment.
Just left a few comments! I'm still learning the codebase, so feel free to ignore.
core/src/odbc_uri.rs
Outdated
| None | ||
| } else { | ||
| Some(x.clone()) | ||
| macro_rules! handle_auth_mechanism { |
There was a problem hiding this comment.
Should this just be a mutable method on ClientOptions? It seems as though it is self-contained enough.
There was a problem hiding this comment.
Oh, I see that we don't control ClientOptions. Maybe a newtype might be nice here to keep things organized.
nbagnard
left a comment
nbagnard
left a comment
There was a problem hiding this comment.
Your change makes sense, but I'd love to have the same logic used everywhere if possible.
It was hard to explain here as a comment, so I made the changes in a branch on my fork. I extracted the logic in a helper and use it to finalize all client_options instance.
I tried a patch patch and it's Ok.
Since we have a customer willing to try it, it might be worth doing the change now, rather than backlog it.
WDYT?
core/src/odbc_uri.rs
Outdated
| }; | ||
| let mut client_options = parse_func().await.map_err(Error::InvalidClientOptions)?; | ||
|
|
||
| // the dns resolver is a private, unchangable field in ClientOptions, so we create a second set |
There was a problem hiding this comment.
We technically only need this on Windows
Co-authored-by: Nicholas Cioli <[email protected]>
nbagnard
left a comment
nbagnard
left a comment
There was a problem hiding this comment.
LGTM!
Just added a couple of tiny suggestions for a few comments.
Co-authored-by: Natacha Bagnard <[email protected]>
Co-authored-by: Natacha Bagnard <[email protected]>
Co-authored-by: Natacha Bagnard <[email protected]>
3 participants
No description provided.