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

Default image now respects cache-control headers #569

Closed
wants to merge 1 commit into from

Conversation

nicolasbuch
Copy link

@nicolasbuch nicolasbuch commented Aug 20, 2024

Issue #, if available:
#563

Description of changes:
The default image now respects cache control headers if set. Otherwise default to "max-age=31536000,public".
Changes have been deployed and tested. Works as intended and solves #563

Checklist

  • 👋 I have run the unit tests, and all unit tests have passed.
  • 👋 I have added unit tests for all code changes.
  • ⚠️ This pull request might incur a breaking change.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@simonkrol
Copy link
Member

Hi @nicolasbuch,

I think this generally looks good, my only thought is that this wouldn't take into account a CacheControl header that was included as an override as part of a b64 encoded request body.

image

That aspect might be difficult to implement since we extract custom headers in the try portion of the try-catch that returns the fallback image.

Either way, we'll continue to evaluate and I'll aim to get this functionality included in the next minor release.

Thanks for your contributions,
Simon

@simonkrol
Copy link
Member

Hi @nicolasbuch,

We've gone ahead and included this functionality internally. You should see it in the template alongside the next minor release. Due to our internal processes, you won't see this PR get merged, though once the changes are in, you'll see a link to this PR and your profile included at the bottom of the README, and this PR will be closed.

Thanks for your contributions to SIH,
Simon

@nicolasbuch
Copy link
Author

Hi @simonkrol

This sounds awesome! Can't wait to roll this out to our customers.
Thanks for your swift handling of the issue and thank you, to you and the team, for maintaining such a great service.

Regards
Nicolas

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

Successfully merging this pull request may close these issues.

2 participants