Skip to content

[BD-14] Install the blockstore app into edx-platform, behind a waffle switch (nutmeg backport)#30322

Merged
kdmccormick merged 2 commits intoopenedx:open-release/nutmeg.masterfrom
open-craft:jill/TNL-8746-nutmeg
May 11, 2022
Merged

[BD-14] Install the blockstore app into edx-platform, behind a waffle switch (nutmeg backport)#30322
kdmccormick merged 2 commits intoopenedx:open-release/nutmeg.masterfrom
open-craft:jill/TNL-8746-nutmeg

Conversation

@pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Apr 27, 2022

Description

Backports #29779 to Nutmeg. See this OSPR for supporting information.

(cherry picked from commit d2e82b7)

Testing instructions

Sandbox (provisioning):

This was a clean backport, so implementation details and testing instructions are the same as for the original #29779

Deadline

ASAP -- @pdpinch / MIT need this in Nutmeg.

Other information

Anyone who is migrating from a standalone blockstore IDA to the platform-integrated blockstore will need to make significant amount of configuration changes in order to avoid losing data. See #29779 for details.

Anyone running blockstore for the first time in production only needs to configure the file storage for bundle files. See blockstore settings.

[BD-14] Install the blockstore app into edx-platform, behind a waffle switch

(cherry picked from commit d2e82b7)
@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Apr 27, 2022
@openedx-webhooks
Copy link

Thanks for the pull request, @pomegranited! I've created BLENDED-1219 to keep track of it in Jira. More details are on the BD-14 project page.

When this pull request is ready, tag your edX technical lead.

@pomegranited
Copy link
Contributor Author

pomegranited commented Apr 27, 2022

@pdpinch Do you want to review? :)

FYI @kdmccormick @bszabo @bradenmacdonald

@e0d
Copy link
Contributor

e0d commented May 6, 2022

@ormsbee @pomegranited Can you chime in on whether you think this is critical to merge this for phase 3?

@pomegranited
Copy link
Contributor Author

@e0d Yes, I think it is. We'd planned to get the original PR merged prior to the nutmeg cut, but didn't make the deadline.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the backport!

✅ I confirmed that the diff is essentially identical to the one in the original PR. (Heads up -- I didn't do in-depth review of the changes themselves.)

✅ I browsed the diff between the base of the original PR and the base of this PR to ensure there are no differences that might conflict.

❌ I found that running make requirements on this branch yields:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
blockstore 1.2.1 requires drf-nested-routers, which is not installed.

I was able to work around this by adding drf-nested-routers to base.txt, but I think make upgrade needs to be run in order to make sure we're properly pulling blockstore's installation dependencies into edx-platform. I imagine this is a problem on the original PR too.

✅ l ensured that, with that ^ workaround, LMS and CMS can be run in Tutor with this branch, and I can see the Blockstore section of the CMS admin console 🎉

❌ When I try to add a new Bundle, I am not asked which Collection it should be a part of. When I save the Bbundle, I see a 500: django.db.utils.IntegrityError: (1048, "Column 'collection_id' cannot be null")

@pomegranited
Copy link
Contributor Author

Thank you for your review and testing @kdmccormick !

❌ I found that running make requirements on this branch yields: ...blockstore 1.2.1 requires drf-nested-routers, which is not installed.

Good catch! I ran make upgrade and pushed the result, see 32541e7. Will redeploy my sandbox too.

❌ When I try to add a new Bundle, I am not asked which Collection it should be a part of. When I save the Bbundle, I see a 500: django.db.utils.IntegrityError: (1048, "Column 'collection_id' cannot be null")

I don't think we ever add Bundles directly in Django Admin.. LabXchange uses the Content Library APIs (and has a dedicated collection UUID for this), or you can use the Content Libraries Authoring Frontend. This MFE is also enabled on the sandbox, you can use the edx@example.com / edx user to login to the LMS, and then view Studio and navigate to the Libraries tab.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe make upgrade was a bad suggestion. Since we're on nutmeg.master, it might have pulled in new package versions that aren't compatible with nutmeg.master, hence the build breakage.

I know this would be bad practice for the master branch, but I wouldn't be opposed if you were to just add the specific new packages needed (drf-nested-routers and django-environ, I think) to the .txt requirement files, such that make requirements works again.

(As an aside: I think this didn't break on master because many deployers run pip install -r requirements/edx/base.txt instead of make requirements, the former of which will happily install any missing requirements. Since make upgrade is run for edx-platform on a weekly basis, the make requirements breakage probably went unnoticed).

This should be automatically added by blockstore, but wasn't.
@pomegranited pomegranited force-pushed the jill/TNL-8746-nutmeg branch from 32541e7 to 961591f Compare May 11, 2022 01:33
@pomegranited
Copy link
Contributor Author

I know this would be bad practice for the master branch, but I wouldn't be opposed if you were to just add the specific new packages needed (drf-nested-routers and django-environ, I think) to the .txt requirement files, such that make requirements works again.

Cool, thanks for that @kdmccormick !

django-environ was already in there, so I reverted the previous change and manually added drf-nested-routers.

(As an aside: I think this didn't break on master because many deployers run pip install -r requirements/edx/base.txt instead of make requirements, the former of which will happily install any missing requirements..

Yep.. our sandboxes do, since they still run the edxapp ansible role, which just installs each requirements file one by one.

🤞 this one passes!

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! 🚀

@kdmccormick kdmccormick merged commit c089415 into openedx:open-release/nutmeg.master May 11, 2022
@openedx-webhooks
Copy link

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@pomegranited pomegranited deleted the jill/TNL-8746-nutmeg branch May 12, 2022 00:16
@pomegranited
Copy link
Contributor Author

Thank you @kdmccormick ! FYI @pdpinch

@kdmccormick
Copy link
Member

No problem!

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

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants