Conversation
| impl RegistryClientBuilder<'_> { | ||
| pub fn new(cache: Cache) -> Self { | ||
| impl<'a> RegistryClientBuilder<'a> { | ||
| pub fn new(base_client_builder: BaseClientBuilder<'a>, cache: Cache) -> Self { |
There was a problem hiding this comment.
An alternative in one direction would be starting with a default base client builder (easier for the tests, easier to get wrong in production code), an alternative in the other direction would be making IndexLocations a parameter here to clarify them as mandatory.
| let client = RegistryClientBuilder::try_from(client_builder)? | ||
| let client = RegistryClientBuilder::new(client_builder.clone(), cache.clone()) |
There was a problem hiding this comment.
This is a logic change as it was previously (accidentally?) using a temporary cache.
| index_urls: IndexUrls::default(), | ||
| index_strategy: IndexStrategy::default(), | ||
| torch_backend: None, | ||
| cache: Cache::temp()?, |
There was a problem hiding this comment.
We avoid this fallible construction by passing the cache in explicitly. This avoids creating temp dirs we drop immediately after.
1c4a25d to
c3cb63f
Compare
| .native_tls(network_settings.native_tls) | ||
| .keyring(keyring_provider) | ||
| .allow_insecure_host(network_settings.allow_insecure_host.clone()); | ||
| let client_builder = client_builder.clone().keyring(keyring_provider); |
There was a problem hiding this comment.
I worry about the friction this adds to adding a keyring provider, i.e., that it makes it easier to forget. I'm not sure what we can do though, it's not substantively worse than the status quo.
There was a problem hiding this comment.
That's a motivation for this change! I want to move towards something that's more consolidated instead of having to remember to add each field to the builder each time. I looked at the keyring quickly, but it seemed to complex to pull out for now.
Alternative to #15105
Instead of building a
BaseClientBuilderfromNetworkSettingseach time we need a client, we instead build a singleBaseClientBuilderand pass it around. TheRegistryClientBuilderthen usesBaseClientBuilderexclusively for configuration. This removes a chunk of copy-and-paste code, and also moves the fallibleretries_from_envinto a single placeBorrow vs. clone is mostly ad-hoc, we can change it in either direction if it matters.
Closes #15105