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

Enable providing endpoint_url through environment variables #704

Merged

Conversation

elephantum
Copy link
Contributor

Currently, if you're using non-AWS S3 endpoint you're stuck with less ergonomic syntax, see #120 (comment) for example.

Sometimes it's feasible, but sometimes it breaks flexibility that fsspec provides.

This simple PR makes endpoint_url a first-class configuration option, alongside with key and secret, which are already supported exactly in the same way.

@elephantum
Copy link
Contributor Author

WIth this code change it is possible to set environment variables to fully control connections to non-AWS S3 buckets, like local minio or S3-compatible store in other clouds.

  • FSSPEC_S3_ENDPOINT_URL
  • FSSPEC_S3_KEY
  • FSSPEC_S3_SECRET

@martindurant
Copy link
Member

Thakns - I suppose this is indeed common enough that it deserves its own top-level argument.

This argument should be documented; ideally, we should specify which once wins in the case that an endpoint is included in both this explicit arg and also in the client_kwargs.

@elephantum
Copy link
Contributor Author

elephantum commented Feb 22, 2023

@martindurant take a look please, I've updated docstring.

Also it seems reasonable to me, to add short article to docs named something like "Connecting to non-AWS buckets", but I do not understand yet how to do it in the correct way.

@martindurant
Copy link
Member

All the docs are in docs/source, a few RST files there. I'm not immediately sure where this would fit in, but there is not too much prose there. I am happy to wait on this PR if you have the time to add something.

@elephantum
Copy link
Contributor Author

elephantum commented Feb 22, 2023

I added some changes to index.rst

If you're ok with the changes, then I think PR is complete.

@martindurant
Copy link
Member

Can you please investigate the failing test, and see whether it is caused by this PR (i.e., does it fail if you check out main branch).

@elephantum
Copy link
Contributor Author

Oh wow, will do

@martindurant
Copy link
Member

______________________________ test_bucket_exists ______________________________

s3 = <s3fs.core.S3FileSystem object at 0x7fa062d2b130>

    def test_bucket_exists(s3):
        assert s3.exists(test_bucket_name)
        assert not s3.exists(test_bucket_name + "x")
        s3 = S3FileSystem(anon=True, client_kwargs={"endpoint_url": endpoint_uri})
>       assert s3.exists(test_bucket_name)
E       AssertionError: assert False
E        +  where False = <bound method S3FileSystem._exists of <s3fs.core.S3FileSystem object at 0x7fa062d2b130>>('test')
E        +    where <bound method S3FileSystem._exists of <s3fs.core.S3FileSystem object at 0x7fa062d2b130>> = <s3fs.core.S3FileSystem object at 0x7fa062d2b130>.exists

@elephantum
Copy link
Contributor Author

Yeah, I did not get to the bottom of it yet.

@elephantum
Copy link
Contributor Author

Fixed. That was an actual bug on my side. Thanks so much for extensive tests

@martindurant
Copy link
Member

Glad we caught it :)

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

2 participants