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

Return correct error when creating a bucket with empty CreateBucketConfiguration #3079

Conversation

dawngerpony
Copy link
Contributor

@dawngerpony dawngerpony commented Jun 19, 2020

Problem

  1. See problem described in create_bucket should fail with CreateBucketConfiguration={} #2210.
  2. One of the other S3 tests (test_streaming_upload_from_file_to_presigned_url) is failing on my machine due to the lack of a region. See test_streaming_upload_from_file_to_presigned_url fails #3080 for more information.
  3. Some of the contributor documentation is a little sparse for newbies.

Changes

This PR:

  • updates the contributor documentation
  • fixes the failing S3 test by providing a region
  • adds a test for empty CreateBucketConfiguration
  • returns the correct exception when presented with an empty CreateBucketConfiguration

Notes

  • 🙏 This is my first PR against this codebase, please be gentle!
  • ℹ️ I'm using this as practice because I want to implement Application Autoscaling (Application Autoscaling service #1931).
  • ❓ I'm not sure why test_streaming_upload_from_file_to_presigned_url is failing, it was passing yesterday.

Fixes #2210, fixes #3080

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 94.393% when pulling f78239f on dawngerpony:fix/2210-s3-empty-createbucketconfiguration into 6305f70 on spulec:master.

@dawngerpony dawngerpony marked this pull request as ready for review June 19, 2020 09:52
Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

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

Hi @dawngerpony, this looks good to me. Thanks for your PR, and welcome to Moto!

@bblommers bblommers merged commit 8ce1202 into getmoto:master Jun 19, 2020
@bblommers
Copy link
Collaborator

Let me know if you need any help with the Application Autoscaling btw.

If you want an idea on how to get started with a completely new service, have a look at this PR: #2957 . It's a very clean implementation of the another new service (Managed Blockchain) that was introduced recently, and should be a good starting point.

@dawngerpony dawngerpony deleted the fix/2210-s3-empty-createbucketconfiguration branch June 19, 2020 17:25
@dawngerpony
Copy link
Contributor Author

Thank you for the lovely welcome, @bblommers - I'm glad my PR looked good to you! 😄 Also thank you for the link to the PR, I'll use it as a template when mocking Application Auto Scaling.

@spulec
Copy link
Collaborator

spulec commented Jun 20, 2020

Welcome to the team!

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