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

Fix two S3 bugs #307

Merged
merged 4 commits into from Apr 26, 2019
Merged

Fix two S3 bugs #307

merged 4 commits into from Apr 26, 2019

Conversation

@mpenkov mpenkov merged commit 07a205f into piskvorky:master Apr 26, 2019
@mpenkov mpenkov deleted the fix-mutable branch April 26, 2019 01:30
@luckydonald
Copy link

@mpenkov Hey, any feedback to the questions above?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Apr 26, 2019

Which questions are you referring to? I don't see any in this PR

@luckydonald
Copy link

luckydonald commented Apr 29, 2019

I asked here (It says pending, so maybe that didn't go through):

    #
    # If the default region is not us-east-1, we need to construct our own
    # session.  This is because smart_open will create a session in the default
    # region, which _must_ be us-east-1 for minio to work.

Why is that? Is this specific to minio, or all custom S3 implementations?
Shouldn't that be automatic?

Needing to set that manually feels like it could cause bugs very easily, later on.
My assumption would be it'll be smart and "just work". Maybe smart_open can set the session accordingly?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Apr 29, 2019

I think it's a minio thing, but I don't use it, so my knowledge is limited.

I don't think it's worth including such details in smart_open library for several reasons:

  • Minio is a nonstandard S3 implementation (it's an edge case)
  • it's already possible to set and override the default S3 region via boto3 mechanisms
  • In the worst case, it's possible to override the region via transport_params

@luckydonald
Copy link

luckydonald commented Apr 29, 2019

How did you stumbled upon the fact that it needs to be us-east-1?
That's new to me, I didn't even know that, as it just workedTM for me.
Does that also apply to other S3 vendors out there, like wasabi etc?

@mpenkov
Copy link
Collaborator Author

mpenkov commented Apr 29, 2019

Trial and error. That's another reason why I'm not confident about including it in the library.

No, I haven't tried other S3 vendors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants