Remove sync credentials#551
Conversation
Only used in connect_client, but that should use async ones anyway
kyleknap
left a comment
kyleknap
left a comment
There was a problem hiding this comment.
Hi @jamesmyatt thanks for following up on refactoring the credential resolution. Even though it is technically a breaking change to remove the public sync_credential property, I'm in favor of the change (as you pointed out) sync credentials are no longer used and the project only documents usage of async credentials. So that will help clear up some confusion. Furthermore, previous maintainers were in favor of removing the sync credentials completely once the implementation no longer used sync creds: #132
Overall, from this first pass, the change looks good. For now, I just had a small comment about adding a changelog entry for it. Otherwise, I plan to pull down the change later this week to do a deeper review on it.
| ) | ||
|
|
||
| creds = [self.fs.sync_credential, self.fs.account_key, self.fs.credential] | ||
| creds = [self.fs.credential, self.fs.account_key] |
There was a problem hiding this comment.
Not actionable but just calling this out... Technically this is a change of behavior as before if a credential was provided to the file system a sync_credential would not be set and account_key would thus take precedence over the explicitly provided credential. That being said, this update seems reasonable as:
- It make it consistent with how the filesystem class resolves credentials (e.g. it prioritizes
self.credentialoverself.account_keyno matter what) - In most cases, the container client will already be instantiated from the file system so this code path in general should not be hit unless
connect_clientis explicitly called.
| ): | ||
| ( | ||
| self.credential, | ||
| self.sync_credential, |
There was a problem hiding this comment.
Could we add a changelog entry noting that we removed the sync_credential property as it was unused and label it as **Breaking** similar to other entries?
2 participants
The sync credentials were only created by internal methods, and only used in AzureBlobFile.connect_client, when the async credentials should have been preferred anyway.
Part of refactoring of
do_connect-related logic: #504