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

support signed urls via connection string alone #478

Merged
merged 1 commit into from
Jul 20, 2024

Conversation

shcheklein
Copy link
Contributor

In a lot of cases we have only a connection and no account name, account key. Those could be extracted from the connection string and it's convenient to have it done automatically by the fs itself.

@shcheklein
Copy link
Contributor Author

@martindurant hey, sorry for bothering you here, do you know who is the right person to check this and do a release? thanks!

@martindurant
Copy link
Member

@hayesgb @TomAugspurger - who is maintaining this repo these days?

@martindurant
Copy link
Member

Is it possible to make a test for this change?

@shcheklein
Copy link
Contributor Author

Is it possible to make a test for this change?

@martindurant yep, it has a test already

@shcheklein
Copy link
Contributor Author

@hayesgb @TomAugspurger @martindurant a friendly reminder folks :)

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Jun 28, 2024 via email

Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

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

I don't really do any maintenance on this project, so I hope someone else merges this.

@@ -1541,11 +1542,19 @@ async def _url(
"""
container_name, blob, version_id = self.split_path(path)

if self.connection_string:
args_dict = parse_connection_string(self.connection_string)
Copy link
Member

Choose a reason for hiding this comment

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

OK, I see what's going on here, looks good to me. One assumes the connected client also has this information, but since there's an upstream function to parse it, I don't see why we shouldn't use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, thanks @martindurant for approving ... I hope maintainers will be able to take a look soon :)

@shcheklein
Copy link
Contributor Author

just a friendly reminder folks, it would be great if we can merge this :)

@TomAugspurger TomAugspurger merged commit 7f06dbd into fsspec:main Jul 20, 2024
4 checks passed
@TomAugspurger
Copy link
Contributor

Thanks @shcheklein

@shcheklein
Copy link
Contributor Author

@TomAugspurger could you also trigger a new release please when you have a minute. Thanks!

@TomAugspurger
Copy link
Contributor

2024.7.0 is out on PyPI now: https://pypi.org/project/adlfs/

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.

None yet

3 participants