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

Make AzureBlobFileSystem anon behaviour configurable via env var. #437

Merged
merged 8 commits into from
Apr 13, 2024

Conversation

microft
Copy link
Contributor

@microft microft commented Nov 24, 2023

Making the anon default value configurable from an env var makes integration easier with helm charts that provide features like RBAC in Azure accounts.

In a multi-cloud application I am working on it is impacting that the code needs to change between AWS and Azure.
The default in s3fs is anon=False, in adlfs anon=True.

This PR would allow for configuration only to change without any code change.

It would facilitate integration with https://azure.github.io/azure-workload-identity/docs/quick-start.html without any code change apart from environment variables.

Making the `anon` default value configurable from an env var makes integration easier with helm charts that provide features like RBAC in Azure accounts.
Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. A few more requests:

  1. Add docs for this to the class docstring
  2. Add docs in the README
  3. Add a test

adlfs/spec.py Outdated Show resolved Hide resolved
@TomAugspurger
Copy link
Contributor

Are you able to add a test and docs?

I think pytest has a fixture to monkeypatch environment variables for a test. Just need to confirm that it gets set in __init__ properly.

@TomAugspurger
Copy link
Contributor

Checking in here, will you have a chance to update these last things @microft?

@microft
Copy link
Contributor Author

microft commented Dec 31, 2023

Will try to update in the next few days.
(Festivities plus child birth have kept me mostly offline)

@microft
Copy link
Contributor Author

microft commented Feb 14, 2024

@TomAugspurger I got a test going, but the cachable property (inherited from AbstractFileSystem) seems to complicate testing multiple instances with the same account_name and connection_string. Hence the monkey patch on cachable for that test.
Hope it makes sense.

@r-priyam
Copy link

@TomAugspurger: Any idea when this can be approved and merged, please? It's affecting us badly, too.

@TomAugspurger TomAugspurger merged commit 3005f6e into fsspec:main Apr 13, 2024
4 checks passed
@TomAugspurger
Copy link
Contributor

Thanks!

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.

4 participants