issue #39: pass use_ssl, verify arguments to boto3 client constructor#40
issue #39: pass use_ssl, verify arguments to boto3 client constructor#40chairmank wants to merge 2 commits intoPyFilesystem:masterfrom chairmank:issue-39-ssl-configuration-parameters
Conversation
| aws_secret_access_key=self.aws_secret_access_key, | ||
| aws_session_token=self.aws_session_token, | ||
| use_ssl=self.use_ssl, | ||
| verify=self.verify, |
There was a problem hiding this comment.
The use_ssl and verify arguments need to be applied below as well, starting on line 410 (in the "client" method).
There was a problem hiding this comment.
oops! thanks for catching my silly oversight. fixed
This commit addresses #39 * Added `use_ssl`, `verify` arguments to the `S3FS` `__init__` method, which assigns instance attributes with the same names * Pass these instance attributes to `boto3.resource` in the `s3` property method. * Pass these instance attributes to `boto3.client` in the `client` property method.
The `use_ssl` parameter in the query string is interpreted as follows: * `'use_ssl=1'` or omitting the parameter from the query string means `use_ssl=True` * Any other value means `use_ssl=False` The `verify` parameter in the query string is interpreted as follows: * A non-empty value `'verify=path/to/cert/bundle.pem'` means `verify='path/to/cert/bundle.pem'` * An empty value, `'verify='` means `verify=False` * Omitting the parameter from the query string means `verify=None`, which tells `S3FS` to use the default CA cert bundle that is used by `botocore`
|
Any more changes requested? Sorry if I missed anything. I'm eager to make it right. |
|
This is workable as-is but I would still much prefer a consistent approach: 0 instead of empty string for false on verify. Also, did you see my comment on the main thread regarding ignoring verify altogether if use_ssl is false? That seems to be how s3fs handles things; my environment has an issue with certificate verification and I don't get any error when I disable SSL in that library. Furthermore, boto throws a warning if the two options clash. It seems like they should/will always be set to false together, so why not make the whole thing a bit simpler for the end user? In any case I hope we can wrap this up soon. I've vendored this PR for now in my project and I want to get that out of there as soon as possible. |
@amachanic amachanic For the other parameters that are exclusively boolean,
and I think that this may be the right approach. |
Sorry that I missed that comment on the other thread. I'm concerned that silently discarding the |
|
I just stumbled across this issue, because I ran into a similar situation, where I need to either provide a different CA bundle (according to the docs, could be done via the Besides the consistent API ( Is there any chance we can finalize the PR? |
… constructor implementation slightly changed based on old discussion in PyFilesystem#40
3 participants
This commit addresses #39
use_ssl,verifyarguments to theS3FS__init__method,which assigns instance attributes with the same names
boto3.resourcein thes3property method.
boto3.clientin theclientproperty method.