-
Notifications
You must be signed in to change notification settings - Fork 104
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
Fix account host #480
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, ...)
There was a problem hiding this comment.
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.
Thanks! |
Added account_host parameter to AzureBlobFileSystem, which is needed for package to work in other Azure clouds.