-
Notifications
You must be signed in to change notification settings - Fork 2
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
Upgraded wagtail to 2.9.3, added image rendition caching #2029
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2029 +/- ##
==========================================
+ Coverage 87.96% 97.10% +9.14%
==========================================
Files 328 157 -171
Lines 14663 4912 -9751
Branches 1024 0 -1024
==========================================
- Hits 12898 4770 -8128
+ Misses 1529 142 -1387
+ Partials 236 0 -236 Continue to review full report at Codecov.
|
dependencies = [("cms", "0028_course_program_index_pages")] | ||
dependencies = [ | ||
("cms", "0028_course_program_index_pages"), | ||
("wagtailcore", "0043_lock_fields"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason for adding this?
I am getting incosistent history error.
web_1 | django.db.migrations.exceptions.InconsistentMigrationHistory: Migration cms.0029_setup_course_program_index_pages is applied before its dependency wagtailcore.0043_lock_fields on database 'default'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to account for a field name change in one of the Wagtail models, but I forgot to test in cases where you're not migrating from zero. I think I need to change this so it just uses some if/else logic to refer to the model field rather than adding that migration as a dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codewise looks good but could not test because of migrations inconsistent history.
189ccbd
to
c456644
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pre-Flight checklist
app.json
What are the relevant tickets?
Partially addresses #2019
What's this PR do?
Upgrades Wagtail to 2.9.3, which includes image rendition caching, and adds configuration to perform that caching
How should this be manually tested?
Since this involves a bunch of library upgrades, a good site-wide smoke test won't hurt.
To test the caching specifically:
Any background context you want to provide?
We are making an effort to improve caching of these images in Fastly. In an ideal world, these images wouldn't end up in the Redis cache at all. This can be considered a fallback in case the caching in Fastly falls through. If Wagtail renders an image, it will automatically be saved in the Redis cache. Subsequent requests for the image will be served from there, saving a significant amount of time/memory