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

Avoid use of private APIs from azure.storage #427

Merged
merged 4 commits into from
Sep 16, 2023
Merged

Avoid use of private APIs from azure.storage #427

merged 4 commits into from
Sep 16, 2023

Conversation

benrutter
Copy link
Contributor

@benrutter benrutter commented Sep 14, 2023

This is designed to fix issue 426, and remove use of private modules from azure.storage - hopefully help avoid future issues like the recent sdk_moniker key error cropping in.

There's two changes here, both of which are pretty minor, but the second of which might need some discussion on whether it's the right approach.

Import switches

BlobPrefix, BlobBlock, BlobPoperties and BlobType are all exposed as importables straight from azure.storage.blob without needing the private modules, so this is a drop in replacement.

Default Block Size

This change might need a little more discussion - I've hardcoded it here to avoid using create_configuration, which @jalauzon-msft suggested as a possibility, but that means it won't automatically change if azure shifts it in future.

The alternative approaches I can think of are:

  • Continue using create_configuration, which would be responsive to changes from azure, but also potentially lead to issues in future since it is a private method.
  • Open an issue with azure SDK to add default block size as a supported importable constant (and either leave as is or hardcode in the meantime)

@benrutter
Copy link
Contributor Author

Looks like I did something unexpected since all the tests are now failing - not sure I understand where the errors are coming from though? (I've checked, and the new imports are definitely just extended ways of pulling from the initially private modules, so I don't think I've accidently switched out an azure object for another one)

@TomAugspurger
Copy link
Contributor

Thanks @benrutter! Agreed with hardcoding the default block size and getting that from a public API if and when it's added to the azure SDK.

I wonder if the failing tests are fixed by #422. I'll take a closer look later in the week at these two PRs.

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Sep 16, 2023

Hmm, seems that I can't push to your branch @benrutter. Are you able to merge main and update this PR?

Edit: never mind, seems I had something misconfigured locally.

@TomAugspurger
Copy link
Contributor

78af9d1 should fix the CI failure (sync vs. async BlobPrefix)

@TomAugspurger TomAugspurger reopened this Sep 16, 2023
@TomAugspurger TomAugspurger merged commit 49ed5f2 into fsspec:main Sep 16, 2023
5 of 8 checks passed
@TomAugspurger
Copy link
Contributor

Thanks!

@benrutter
Copy link
Contributor Author

Thanks @TomAugspurger - just saw your messages now!

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

Successfully merging this pull request may close these issues.

Avoid private APIs from azure.storage
2 participants