Upload all content to s3 as public and immutable #1871
Conversation
|
r? @sgrif (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
@jtescher Thanks for taking this on!
The uploaders is currently used for three purposes:
- Upload rendered readme files.
- Upload crates.
- Upload database dumps.
The former two use the crate name and version in the path, so the content is indeed immutable. The latter, however, uses the same path every day, so marking it as immutable would be wrong.
I think the right solution for the database dumps would be to use the "Expires" header rather than "Cache-Control", and let it expire at approximately the time the next database dump is expected to be uploaded.
I believe that in the absence of any caching-related headers, CloudFront caches files for 24 hours by default. For the database dumps this can already be too long, so this is actually an existing issue unrelated to the ticket you are working on, so feel free to consider it out of scope. In any case, we need to excempt the database dumps from being marked as immutable and cached for a full year.
|
Ah thanks for the context @smarnach! I'll update this to set the appropriate headers for both scenarios. |
jtescher
force-pushed
the
imutable-s3-uploads
branch
from
faf1ff7 to
dc3a221
Compare
|
@smarnach I reworked the PR to be more conservative, currently only marking crate data immutable. It seems like rendered readme file data may also be altered at a later point via render-readmes.rs (or are the cloudfront caches purged currently when this occurs? If so I can add the header there too) If you know the frequency at which the database dump job is run I'm happy to add the expires header as well. |
There was a problem hiding this comment.
Thanks for the updates! You are right that we may want to regenerate the readmes if we update the rendering code, so we shouldn't mark them as immutable. We could probably cache them for more than 24 hours, but let's go one step at a time.
The code in this PR looks good to me now, but I'd like to manually test it in a production-like setup first before merging. I'm not sure when I will have time to do so – it will probably have to wait a week or two.
|
I tested this on a setup similar to production, and everything is working as expected. There are two things currently blocking me from merging this:
|
|
@bors r+ |
|
📌 Commit dc3a221 has been approved by |
Upload all content to s3 as public and immutable Adds cache control header with value `public,max-age=31536000,immutable` to all future crate and README uploads. Fixes #1826
|
☀️ Test successful - checks-travis |
Adds cache control header with value
public,max-age=31536000,immutableto all future crate and README uploads.Fixes #1826