-
Notifications
You must be signed in to change notification settings - Fork 20
[WIP] Revert "Revert "Upgrade blockstore to use Python 3.8 and Django 2.2+. (#82)"" #86
[WIP] Revert "Revert "Upgrade blockstore to use Python 3.8 and Django 2.2+. (#82)"" #86
Conversation
Thanks for the pull request, @Kelketek! I've created OSPR-4978 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
01ee033
to
9a4dc98
Compare
@bradenmacdonald @kdmccormick This is ready for another go. To keep things clear I've kept the original changes in their own commit and have put new stuff in its own commit. |
@Kelketek Thank you for your contribution. Please let me know once it is ready for our review. |
@natabene I think it may already be-- this is sort of a redux of #82 with @kdmccormick 's notes addressed. I don't think we have a specific procedure for how to handle a PR that was reverted and had a few notes, but it seems most logically sensible to me to just continue where the review left off when creating the new PR. |
Nah don't worry about it. |
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.
👍 Thanks! Conditional approval if you fix the minor -test
issue I pointed out. Sorry for originally recommending removing the 3.8 tests :-/
- I tested this: as described, on devstack - thanks for clear instructions!
- I read through the code
- I checked for accessibility issues
- Includes documentation
@bradenmacdonald Feel free to merge whenever your comment is addressed. Blockstore isn't continuously deployed, so we can still deploy the May->September today and deploy this PR another day (although, this doesn't look very risky, so if you wanted to deploy all at once, that's cool too). Merging the configuration PR and then deploying is what will trigger the actual Py35->Py38 upgrade. |
@bradenmacdonald This should be good to go now. Made that change and ran through the commands again to make sure it all works smoothly. |
Thanks @kdmccormick! Merging now. Let's deploy this tomorrow or so? |
@Kelketek 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Actually @kdmccormick let's wait until Wednesday please. Apparently some important stuff is happening on LX prod tomorrow so want to keep risk low. |
Given all the ops wackiness if the past couple of days, did you still want this rolled out tomorrow @bradenmacdonald? |
@kdmccormick Let's aim for Thursday, hopefully things are calmer. Thanks! |
Sounds good! I'll aim for 10am ET tomorrow. |
@bradenmacdonald @Kelketek This has been in production for a day, and we haven't seen any issues. Ping me over on https://github.com/edx/configuration/pull/5913 when you're ready for the next steps (merge, redeploy Stage with 3.8, smoke test, redeploy Prod with 3.8, monitor). I am good to go to Stage at any point, but I'd prefer not to go to Prod until Monday. |
This PR rolls back the rollback of Python 3.8 compatibility, while changing the requirements to allow 3.5 to still run and with the added ability to run both Python versions in the devstack by specifying the variable when invoking make.
Merge deadline: Soon's good.
Testing instructions:
Blow away your cached images for blockstore and the cached volumes. If you're feeling frisky, do
make dev.up
in devstack to make sure your other devstack containers are in active use, make sure blockstore is stopped, and rundocker system prune
to blow away everything else. If you've got other containers you care about, don't do that. Instead, the following may be enough:This command will probably error out, giving you a list of volumes that can't be destroyed because they're attached to containers.
docker kill
, thendocker rm
each container hash it mentions, then run the above command again.You may need to manually build both versions of the devstack to make sure old copies aren't present:
Now you can launch makeserver or easyserver by specifying an environment variable. For instance:
...or
And, of course, other relevent commands are also updated:
From within each shell, you'll want to run
Unfortunately the containers can't have the same name. Sad. So if you want to run the integration tests with 3.5 (3.8 is the new default), you'll need to use a modified version of the integration test commands:
Author notes and concerns:
I don't know if it's worth updating the README for all of this since we don't expect it to last long and we expect most people to be developing on 3.8 in the interim, which the Makefile will use by default. But I can if needed.
Reviewers