-
Notifications
You must be signed in to change notification settings - Fork 73
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
feat: enable both libraries v1 and v2 in production by default [FC-0062] #1329
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @pomegranited! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1329 +/- ##
=======================================
Coverage 92.73% 92.73%
=======================================
Files 1035 1035
Lines 19259 19259
Branches 4069 4069
=======================================
Hits 17859 17859
Misses 1335 1335
Partials 65 65 ☔ View full report in Codecov by Sentry. |
@jristau1984 CC @KristinAoki We are planning to merge this PR soon to turn on the new content libraries v2 features by default, for the upcoming Sumac release. But I imagine that 2U may wish to hold off on launching this feature (?). I'd like to check if that's the case. If so, could you please override the LIBRARY_MODE="v1 only" will look exactly the same as it does today LIBRARY_MODE="mixed", the new default, would look something like this: |
@bradenmacdonald - Will there be a waffleflag for this experience, so that we can release it in a controlled way for testing/migration? |
Tagging @kdmccormick and @ormsbee. I realized this will actually be more complicated and we might be jumping the gun a bit. First, 2U can't turn this on anytime soon because it requires Meilisearch. For those deploying with Tutor, it's easy to get that set up, but obviously it's a bigger lift for 2U. Plus the current Second, we can think about a waffle flag to turn it on only for certain groups of users. Since that's a backend thing it might also make sense to detect if Meilisearch is configured, and default to OFF (v1 only) in that case or otherwise ON (mixed). Then we achieve our goal of enabling it by default for the release while not prematurely launching it on edx.org nor for MIT. Third, we haven't implemented permissions yet so this is mostly useful for global staff - but we'll be trying to add that ASAP. |
Noted -- I've moved this to Draft to avoid merging too soon.
There isn't yet, but there can be.. I can refactor out this
Product have asked us to turn v2 libraries on by default if Meilisearch is enabled. But I think it would be better for operators and more predictable to implement if we have the v2 libraries waffle flag off by default, and spend time documenting everything operators need to do before they can enable this feature, and let them turn it on manually.
Ach good point -- created openedx/modular-learning#235.
That functionality is baked into waffle flags, and so the dynamic MFE config hit should take care of that for us?
Working on this in the next 2 weeks: #1342 |
@pomegranited I think replaced LIBRARY_MODE with two Waffle flags is a great idea. Regarding the toggles:
In vanilla Open edX, it's impossible to default a waffle flag to ON. The way we default-ON a bunch of waffle flags is, unfortunately, with a big ol tutor-mfe init script. Could I suggest instead having to opt-out toggles? If we tell 2U and MIT this now, then they can stick
|
Ah, on second thought, that is a good point @pomegranited . Maybe the New Libraries flag needs to remain opt-in. Regardless, +1 on the two-waffles idea. |
@kdmccormick @pomegranited I disagree, and I want to have it so that (1) Tutor installs and configures Meilisearch by default (the plugin code is moved into the core, just like for Elasticsearch), and (2) v2 libraries beta is on by default and admins must opt out. The reason is that we saw with Redwood, almost nobody has tried out any of the new taxonomy features or course search features if they require even the slightest effort to set up. |
@bradenmacdonald I am on board with that. Two opt-out waffles, then? |
Description
Changes the
LIBRARY_MODE
default in the production environment to"mixed"
, to show legacy v1 Libraries alongside v2 Libraries.This change impacts Course/Library Authors -- previously only legacy v1 Libraries were enabled by default.
Supporting information
Relates to: #1324
Private-ref: FAL-3858
Testing instructions
No extra manual testing required -- this change makes the production default match development.
Other information