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

Fix account host #480

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Fix account host #480

merged 3 commits into from
Jul 1, 2024

Conversation

dorbaker
Copy link
Contributor

Added account_host parameter to AzureBlobFileSystem, which is needed for package to work in other Azure clouds.

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 the PR, left a few comments. LMK if you have time to look into them. Otherwise, no worries.

I suspect that this will be hard to test. We use azurite for emulating storage which uses its own account URL.

adlfs/spec.py Outdated
@@ -287,6 +293,7 @@ def __init__(
]
self.location_mode = location_mode
self.credential = credential
if account_host: self.account_host = account_host
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably fail our linting on the formatting. You can run either pre-commit run --all-files or black locally to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

adlfs/spec.py Outdated
@@ -235,6 +240,7 @@ class AzureBlobFileSystem(AsyncFileSystem):
def __init__(
self,
account_name: str = None,
account_host: str = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unfortunate, but we'll need to put this new parameter at the end. That way we don't break users using positional args AzureBlobFileSystem(account_name, key, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it the last parameter so it doesn't break anything positional.

adlfs/spec.py Outdated Show resolved Hide resolved
@TomAugspurger TomAugspurger merged commit 126ffc0 into fsspec:main Jul 1, 2024
4 checks passed
@TomAugspurger
Copy link
Contributor

Thanks!

@dorbaker dorbaker deleted the fix-account-host branch July 1, 2024 19:35
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.

2 participants