-
Notifications
You must be signed in to change notification settings - Fork 716
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
Upgrade Python dependencies #12165
Upgrade Python dependencies #12165
Conversation
Build Artifacts
|
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.
The code read-through here seems fine, but I think some regression testing from the QA team would be very helpful @pcenov @radinamatic. It seems like smoke testing all of the various assets would be the place to start per Richard's notes. Not sure @rtibbles if you have any user workflows that would help with confirming redis-related things are working as expected?
The only way to test that would be alongside |
Hi @rtibbles sorry but I am still not sure how to properly test this one - could you please provide additional testing guidance? |
As @marcellamaki mentioned - I think just smoke testing the various assets here is about all that is needed, so just making sure they all start (bar the Mac installer, which we have a separate issue filed for) - and ensuring that you can access Kolibri. |
Hi @rtibbles
lesson.resource.mp4 |
Seems like this is less straightforward than I had hoped! |
Hi @pcenov, I have rebased onto the latest develop, which should fix the 500 error you saw in import, and have made a fix for the error I saw in the Android logs! |
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.
I confirm that now the Android app can be installed however it takes about 2 minutes to get to the first page of the setup wizard during which the user sees only a blank white screen. Here's the logcat log for that:
No other regressions observed in both the app and the desktop version of Kolibri.
Hrm, interesting - I'm not seeing anything unusual in the logs, my only hunch is that this has added a lot of additional files, causing the unpacking to take longer. |
Yeah, sadly that seems to be the case - it just takes a lot of time for the unpacking to happen... |
I can confirm this as replicable on my devices too. If there's nothing we can do to speed up the unpacking time, can we at least make sure to have the Kolibri loader instead of the blank screen for the duration? That would be better UX... |
Or as we mentioned it with @pcenov, straight-up and explicit |
Looking at the assets, it looks like we now have two different packages that provide a complete copy of the timezone database, I think this is the cause of the extra files. I think we can clean this up and only have one of these packages, but it seems simpler to do this after 0.17.0 is released. Bumping this to the first patch milestone instead. |
…e with Python 3.6 and other dependencies.
Hi @rtibbles - I've tested again all the installers without any regressions observed, should be good to go! |
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.
Updated changes make sense to me
6992f95
into
learningequality:release-v0.17.x
Summary
References
Because we are still supporting EOL Python versions, we have to do these upgrades manually rather than relying on dependabot
Reviewer guidance
First we check if the tests pass, then a smoke test of the asset is in order, particularly testing Redis caching.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)