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

Add support for setting custom headers for files uploaded to S3 #7

Merged
merged 4 commits into from Jun 11, 2013
Merged

Add support for setting custom headers for files uploaded to S3 #7

merged 4 commits into from Jun 11, 2013

Conversation

hannseman
Copy link
Contributor

Today there's only support for setting the Cache-Control header. I've removed the S3_CACHE_CONTROL setting and added S3_HEADERS which supports setting whatever header you want:

S3_HEADERS = {
    'Expires': 'Thu, 15 Apr 2010 20:00:00 GMT',
    'Cache-Control': 'max-age=86400',
}

@hannseman
Copy link
Contributor Author

Bump @e-dard :)

I've also got a commit fixing an issue where setting the header Content-Encoding: gzip was set on all files uploaded and not only files with gzippable mime-types:
hannseman/flask-s3@ab3333bad1c5ca47b2820f9802420169fe98e1ba

But we'll take in another PR if you want to merge this one.

@e-dard
Copy link
Owner

e-dard commented Jun 10, 2013

OK, finally got some time to sit down and look at this 😄 I've been completely off-base with Flask for while, as my new gig is mainly a Ruby shop...

Unfortunately I merged the previous PR with too much haste, and I really wish I hadn't 💣. The solution you propose, here, is clearly a much better alternative. However, now I've gone and sullied my once relatively tidy extension, I'm going to have to live with it.

Your PR will break Flask S3 as it stands, because existing deployments may (will) be using S3_CACHE_CONTROL and S3_USE_CACHE_CONTROL, so it's going to need some modifications. Here are my suggestions:

  • put back the cache-based defaults to maintain backwards compatibility;
  • update the documentation for those defaults stating that they are deprecated, and S3_HEADERS should be used instead;
  • On instantiation of the FlaskS3 object use the values of S3_USE_CACHE_CONTROL and S3_CACHE_CONTROL to decide if you need to set those keys in S3_HEADERS.

I will add some line comments to clarify the above.

Don't worry if these changes are too much to ask, I'll get around to them eventually. I promise a quick review if you decide to do them, though 😆

set to `True`.
`S3_USE_CACHE_CONTROL` Specifies whether or not to set the metadata for the
Cache-Control headers.
**Default:** `False`
Copy link
Owner

Choose a reason for hiding this comment

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

For both of these settings I would simply add

Deprecated. Please use S3_HEADERS instead.

@hannseman
Copy link
Contributor Author

No problem! I understand that you need backwards compatibility, somehow I managed to not think about that. I'll give it a shot and come back to you.

See http://developer.yahoo.com/performance/rules.html#expires
for more information.

**Default:** `{}`
=========================== ===================================================
Copy link
Owner

Choose a reason for hiding this comment

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

The code block looks gnarly in the table for some reason. Thinking about it, it may be better just to keep the documentation here to:

Sets custom headers to be sent with each file to S3.
Default: {}

And then after Uploading your Static Assets add another very short section: Setting Custom HTTP Headers, outlining how to use S3_HEADERS (along with relavant links — on that note can you use the foo_ and .. _foo http://bar.com notation?)

e-dard added a commit that referenced this pull request Jun 11, 2013
Adds support for setting custom HTTP headers, on files uploaded to S3
@e-dard e-dard merged commit a7b8152 into e-dard:master Jun 11, 2013
@e-dard
Copy link
Owner

e-dard commented Jun 11, 2013

Thanks @hannseman!

@hannseman
Copy link
Contributor Author

Thanks for the merge! @e-dard

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.

None yet

2 participants