Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Altered get_bucket to use HEAD. #2082

Closed
wants to merge 2 commits into from

Conversation

toastdriven
Copy link
Contributor

In reaction to #2078 (& the mentioned blog post), this pull alters the behavior of S3Connection.get_bucket. This switches from the old approach of trying to fetch keys in the bucket to utilizing HeadBucket.

Some further notes. I back-ported the old method to GSConnection, since it doesn't appear Google Storage supports HEAD on a bucket - https://developers.google.com/storage/docs/reference-methods. I'd love to be corrected on this, but for now, I copied over the old behavior. I don't have a GS account to test with, so I'm really hoping I didn't break it. 馃槙

Beyond that, this is mostly backward-compatible (& less expensive). The only thing that hurts is that HEAD requests (rightfully) don't include a body, but that makes the creation of S3ResponseError worse because there are no details. I faked in the common cases, but if anyone was parsing this before, they're going to have to alter their code. I've documented how they should approach that.

Review please? @jamesls @danielgtaylor @garnaat @mfschwartz

This is mostly backward-compatible (& less expensive). However, if you were parsing exception messages from this method, you should refer to the documentation on how to change your code.
the bucket exists.

If the default ``validate=True`` is passed, a request is made to the
service to ensure the bucket exists. Prior to Boto v2.25.0, this fetched
Copy link
Member

Choose a reason for hiding this comment

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

maybe use .. versionchanged:: for this?

@jamesls
Copy link
Member

jamesls commented Feb 7, 2014

I would still like to hear what others think about the change, and see if anyone has any other concerns.

The code itself, provided everyone is ok with the changes, looks good to me, so :shipit: to the code.

keys weren't being fetched before.
@toastdriven
Copy link
Contributor Author

I updated the wording on the docs & added the versionchanged declaration. Any other changes? I'm still awaiting internal clarification on pricing & IAM permissions.

@garnaat
Copy link
Member

garnaat commented Feb 7, 2014

I did confirm by experimentation that using HEAD will result in the lower charges compared to GET. Not sure when the HEAD Bucket was added but it wasn't there at the time this code was written. I'm still a little concerned that this is such a commonly used method that this change will have unintended consequences for existing users but the argument for making the change is also very strong. So, +1 from me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants