-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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!: A Better API for Derived Settings #36192
feat!: A Better API for Derived Settings #36192
Conversation
7b56894
to
11dbd37
Compare
@feanil , this is ready for review. @dianakhuang , I have confirmed this PR using the redacted LMS yaml file. If you're able to provide the redacted CMS yaml, I can confirm with that as well. |
@kdmccormick working on a redacted studio file right now, so I'll try to get you one by the end of today. |
Tyty @dianakhuang -- I will keep this open until I hear from you. |
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.
Looks good, much easier to manage.
Confirmed with CMS settings. Will rebase and merge. |
11dbd37
to
2426dc7
Compare
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
A tiny update to the directions in devstack.py to enable theming based on recent changes after #36192 was merged.
Updates theming config example in our docs after recent changes to edx-platform (see openedx/edx-platform#36192).
Description
The Python API for declaring derived settings was confusing to the uninitiated
reader, and also prone to spelling mistakes. This replaces the API with one
that is more readable and more concise, and updates the implementation of
derive_settings
to properly derive settings declared using the new API.BREAKING CHANGE: The
derived
andderived_collection_entry
function arereplaced with the
Derived
class. We do not expect those functions to havebeen used outside of edx-platform, but if they are, this commit will cause them
to loudly ImportError.
Note that there should be NO change in behavior to the
derive_settings
function, which we DO know to be used by some external edx-platform plugins.
Before
After
Supporting info
Part of:
Builds on a previous settings refactoring:
Next up:
derive_settings
to lazy load settings. #36205Testing instructions
First, download prod-like YML files to ensure that this works in a non-Tutor environment...
Then, download and save this to
diff_settings.sh
in the root of edx-platform:Finally, from within a Tutor LMS or CMS shell, run
./diff_settings.sh master kdmccormick/use-derived-tooling
Result should be
"No difference!"