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

Don't use s3 bucket_head to check for bucket existence #315

Merged
merged 1 commit into from
May 18, 2019

Conversation

caboteria
Copy link
Contributor

@caboteria caboteria commented May 18, 2019

This PR fixes #314 .

This code caused problems when you have write permission to an s3
bucket path but not read permission to the bucket root.

This code was added to prevent implicit bucket creation (to fix issue
#154) but it appears that it is no longer needed.

This is about
piskvorky#314

This code caused problems when you have write permission to an s3
bucket path but not read permission to the bucket root.

This code was added to prevent implicit bucket creation (to fix issue
154) but it appears that it is no longer needed.
Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

Could you please add a unit test that demonstrates that the code still behaves correctly with your changes?

@caboteria
Copy link
Contributor Author

test_buffered_writer_wrapper_works in test_s3.py tests the happy path. test_nonexisting_bucket in test_s3.py tests that the proper exception (ValueError) is thrown when the bucket doesn't exist. Props for great test coverage!

@mpenkov mpenkov merged commit 5f730f2 into piskvorky:master May 18, 2019
@mpenkov
Copy link
Collaborator

mpenkov commented May 18, 2019

OK, thank you, and congrats on your first contribution to smart_open! 🥇

@caboteria caboteria deleted the fewer-permissions branch May 18, 2019 19:21
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.

writing to S3 requires unnecessary permissions
2 participants