-
Notifications
You must be signed in to change notification settings - Fork 55
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 use_ssl, verify arguments to boto3.client.__init__ #39
Comments
Will look in to that. Would also accept a PR. Btw, I had a problem with 'Minio', which claims to be S3-compatible, but had a number of differences. So I'd like to add a disclaimer that I can't guarantee that anything other than S3 will work properly. |
I just came here searching for this exact fix! Good timing. Any possibility of passing the use_ssl argument via an opener? Edit: I think I meant "FS URL" rather than "opener." I just read the docs and see the ?key=value syntax. Is that how this would work? And one more edit -- apologies for not testing before writing! I tried @chairmank's fix and can confirm that the arguments are passed to the boto constructor from the keys specified in my URL as asked above. But there is an issue: When passing verify=False I'm hitting an exception inside of botocore: SSLError, No such file or directory. This is because "False" is being treated as a string, not a boolean, which tells botocore to treat it as a path, rather than stop verification altogether. I fixed this by changing line 301 of _s3fs.py to: self.verify = False if verify == "False" else verify ... but I'm not sure if there's a better way to handle this. |
@amachanic Did you pass
directly to |
@chairmank I passed it in the URL. Also just discovered that the same problem occurs with use_ssl in a URL; both need to be cast as boolean, as necessary, at some point along the chain. Thanks for doing this fix! |
Actually, this is tricky because query strings are stringly-typed. I think that Interpreting Given these problems, I propose the following:
This feels kludgy. Suggestions for improvement? |
There's a boolean in there already called Other booleans should probably adopt the same convention. Or we pick a better convention. This might be a good time to hammer it out, because I'm going release An alternative would be to have the mere presence of a boolean value considered the True case, and its absense the False case. Possibly there is no elegant solution here, and I'll just pick one and document it. But that would be fine to. |
1 being True is a reasonable choice in my opinion, but it will not work with verify as there is only a False option or a string. So a 0 will have to be introduced. Seems simple and effective enough? That said I do appreciate being able to pass the strings "true" and "false" for consistency, and in those cases it would be ideal to simply support any/all possible casing. The only downside for verify would be someone who wanted to use e.g. "TRUE" or "FALSE" as a file name, but that seems unlikely. |
With regards to verify, I think it might be prudent to separate that in to two parameters. One for the boolean and one for the path to the certs. I'm also not keen on the 'inherit defaults' option of |
Okay, what about the following:
|
Does use_ssl = None mean True inside of boto? If not the default needs to be True in order to not break existing implementations. |
Sorry, I confused the defaultsin my last comment. The defaults in boto, which I propose that
|
I amended my PR. New commit is e53676d |
Additional thought on this: If use_ssl is False, there is no point in verifying the certificate. So in that case, ignore verify altogether and default it to False as well. Convenience feature. |
I would like
S3FS
to pass through a couple of configuration parameters toboto3.client.__init__
(https://boto3.readthedocs.io/en/latest/_modules/boto3/session.html#Session.client):The reason for this change is to support users of S3-compatible storage outside of AWS that require SSL configuration.
It seems like this will be a straightforward addition to the
__init__
method and a corresponding change in theclient
property. I can open a pull request.The text was updated successfully, but these errors were encountered: