Skip to content

Simplify edx-platform settings #36215

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.

Sign up for GitHub

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

Jump to bottom
Open
kdmccormick opened this issue Feb 4, 2025 · 4 comments
Open

Simplify edx-platform settings #36215

kdmccormick opened this issue Feb 4, 2025 · 4 comments
Assignees
@feanil @kdmccormick
Labels
code health Proactive technical investment via refactorings, removals, etc. documentation Relates to documentation improvements epic Large unit of work, consisting of multiple tasks

Comments

@kdmccormick
Copy link
Member

kdmccormick commented Feb 4, 2025 •
edited
Loading

Intended final settings structure

See https://github.com/openedx/edx-platform/blob/master/docs/decisions/0022-settings-simplification.rst#target-settings-structure-for-edx-platform

### Tasks
- [ ] https://github.com/openedx/edx-platform/pull/36115
- [ ] https://github.com/openedx/edx-platform/pull/36131
- [ ] https://github.com/openedx/edx-platform/pull/36162
- [ ] https://github.com/openedx/edx-platform/pull/36188
- [ ] https://github.com/openedx/edx-platform/pull/36124
- [ ] https://github.com/openedx/edx-platform/pull/36132
- [ ] https://github.com/openedx/edx-platform/pull/36192
- [ ] https://github.com/openedx/edx-platform/pull/36205
- [ ] https://github.com/openedx/edx-platform/pull/36224
- [ ] https://github.com/openedx/edx-platform/pull/36225
- [ ] Clean up cms/envs/production cruft (like we did for lms/envs/production)
- [ ] https://github.com/openedx/edx-platform/pull/36306
- [ ] Make *initial* openedx/envs/common, using imported settings in cms/envs/common into openedx/envs/common. Ensure code annotations are loaded for generated settings docs.
- [ ] Move settings up from (cms,lms)/envs/production -> (cms,lms,openedx)/envs/common.
- [ ] Merge any remaining (lms,cms)/envs/common definitions  -> openedx/envs/common.
- [ ] Make (lms,cms)/envs/development.py and update Bare Metal guide. Warn community that devstack.py will actually go away in one release.
- [ ] Ensure (lms,cms)/envs/common actually have good production defaults

Related / nice to have

  • De-crazy the logging settings overrides.
  • De-crazy the MODULESTORE settings conversions.
  • Collapse FEATURES dict
  • Warn/fail when "required" common settings, e.g. secrets, aren't overridden.

Open questions

  • When exactly do we "rebase" Tutor's settings onto (lms,cms)/envs/(common,development)? Most likely, we should do it all at once when those four files are ready, as Tutor's settings are all coupled together into a Jinja hierarchy.
  • Should the settings object should have same type signature between LMS and CMS?

Potential Follow-on Work
@kdmccormick kdmccormick mentioned this issue Feb 4, 2025
Open
@kdmccormick
Copy link
Member Author

@robrap , @dianakhuang -- I have split this off from the greater Let's Talk About OEP-45 issue. We'll be tracking the ongoing edx-platform settings simplification here.

@kdmccormick kdmccormick added epic Large unit of work, consisting of multiple tasks code health Proactive technical investment via refactorings, removals, etc. documentation Relates to documentation improvements labels Feb 4, 2025
@kdmccormick kdmccormick mentioned this issue Feb 4, 2025
Merged
kdmccormick added a commit that referenced this issue Feb 4, 2025
@kdmccormick
feat!: A Better API for Derived Settings (#36192)
3227566 
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` and `derived_collection_entry` function are
replaced with the `Derived` class. We do not expect those functions to have
been 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.

Part of: #36215
@kdmccormick kdmccormick mentioned this issue Feb 4, 2025
Merged
kdmccormick added a commit that referenced this issue Feb 5, 2025
@kdmccormick @feanil
refactor: Use derive_settings to lazy load settings. (#36205)
e1a8b52 
Some of our settings depend on the values of other settings.  Rather
than explicitly looking up each one in the YAML settings file, we can
simply derive them based on the setting in the YAML file after all the
YAML settings have been loaded.

Part of: #36215
Co-Authored-By: Feanil Patel <[email protected]>
@kdmccormick kdmccormick mentioned this issue Feb 6, 2025
Open
@kdmccormick
Copy link
Member Author

Ready for review: #36224

@kdmccormick kdmccormick mentioned this issue Feb 14, 2025
Closed
@kdmccormick kdmccormick marked this as a duplicate of #34446 Feb 14, 2025
This was referenced Feb 14, 2025
Closed
Open
jciasenza pushed a commit to jciasenza/edx-platform that referenced this issue Feb 25, 2025
@kdmccormick @jciasenza
feat!: A Better API for Derived Settings (openedx#36192)
d0b817f 
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` and `derived_collection_entry` function are
replaced with the `Derived` class. We do not expect those functions to have
been 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.

Part of: openedx#36215
jciasenza pushed a commit to jciasenza/edx-platform that referenced this issue Feb 25, 2025
@kdmccormick @feanil @jciasenza
refactor: Use derive_settings to lazy load settings. (openedx#36205)
4197272 
Some of our settings depend on the values of other settings.  Rather
than explicitly looking up each one in the YAML settings file, we can
simply derive them based on the setting in the YAML file after all the
YAML settings have been loaded.

Part of: openedx#36215
Co-Authored-By: Feanil Patel <[email protected]>
@blarghmatey
Copy link
Contributor

I'm not sure if it's too late in the game, but an approach that I am planning to take for managing the settings in my edX deployments is to develop a plugin that takes advantage of Pydantic models and either includes or vendors the logic from pydjantic. That would allow for easily managing the override of any settings via any of environment variables, YAML fragments, JSON, etc. If that could be the approach taken in the Open edX projects directly then it would save me the effort :)

kdmccormick added a commit that referenced this issue Feb 27, 2025
@kdmccormick
docs: Settings Simplification ADR (#36224)
2dc4dfe 
Part of: #36215

Follow up in: openedx/open-edx-proposals#684
@feanil
Copy link
Contributor

feanil commented Feb 28, 2025

@blarghmatey do you have any examples for this? I'm curious about how this would work.

@kdmccormick kdmccormick mentioned this issue Feb 28, 2025
Merged
kdmccormick added a commit that referenced this issue Mar 5, 2025
@kdmccormick
refactor: Clean up CMS production settings cruft (#36306)
774b7e9 
This is a pure refactoring of cms/envs/production.py, removing several
redundant statements that have accrued over the years as the platform moved
from python-only, to python+json, to python+json+yaml, to today's python+yaml
setup.

This is the CMS version of:
* a81493c
* (originally 1593923)

Also included:

* Add some more explicit structure to the both LMS's and CMS's
  production.py using big comments.

* In both LMS and CMS settings, alphabetize the production overrides,
  and remove the extraneous comments. Separate out the handful of settings
  which have useful comments. The rest of the settings' comments were not
  helpful--they were either just stating the obvious, or they were duplicative
  of what's documented in common.py.

Co-Authored-By: Feanil Patel <[email protected]>

Part of: #36215
tonybusa pushed a commit to tonybusa/edx-platform that referenced this issue Apr 23, 2025
@kdmccormick @tonybusa
feat!: A Better API for Derived Settings (openedx#36192)
2992013 
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` and `derived_collection_entry` function are
replaced with the `Derived` class. We do not expect those functions to have
been 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.

Part of: openedx#36215
tonybusa pushed a commit to tonybusa/edx-platform that referenced this issue Apr 23, 2025
@kdmccormick @feanil @tonybusa
refactor: Use derive_settings to lazy load settings. (openedx#36205)
8f03471 
Some of our settings depend on the values of other settings.  Rather
than explicitly looking up each one in the YAML settings file, we can
simply derive them based on the setting in the YAML file after all the
YAML settings have been loaded.

Part of: openedx#36215
Co-Authored-By: Feanil Patel <[email protected]>
tonybusa pushed a commit to tonybusa/edx-platform that referenced this issue Apr 23, 2025
@kdmccormick @tonybusa
docs: Settings Simplification ADR (openedx#36224)
a794de1 
Part of: openedx#36215

Follow up in: openedx/open-edx-proposals#684
tonybusa pushed a commit to tonybusa/edx-platform that referenced this issue Apr 23, 2025
@kdmccormick @tonybusa
refactor: Clean up CMS production settings cruft (openedx#36306)
02d63a7 
This is a pure refactoring of cms/envs/production.py, removing several
redundant statements that have accrued over the years as the platform moved
from python-only, to python+json, to python+json+yaml, to today's python+yaml
setup.

This is the CMS version of:
* a81493c
* (originally 1593923)

Also included:

* Add some more explicit structure to the both LMS's and CMS's
  production.py using big comments.

* In both LMS and CMS settings, alphabetize the production overrides,
  and remove the extraneous comments. Separate out the handful of settings
  which have useful comments. The rest of the settings' comments were not
  helpful--they were either just stating the obvious, or they were duplicative
  of what's documented in common.py.

Co-Authored-By: Feanil Patel <[email protected]>

Part of: openedx#36215
UsamaSadiq pushed a commit that referenced this issue May 14, 2025
@kdmccormick @UsamaSadiq
feat!: A Better API for Derived Settings (#36192)
086066b 
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` and `derived_collection_entry` function are
replaced with the `Derived` class. We do not expect those functions to have
been 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.

Part of: #36215
UsamaSadiq pushed a commit that referenced this issue May 14, 2025
@kdmccormick @feanil @UsamaSadiq
refactor: Use derive_settings to lazy load settings. (#36205)
b076770 
Some of our settings depend on the values of other settings.  Rather
than explicitly looking up each one in the YAML settings file, we can
simply derive them based on the setting in the YAML file after all the
YAML settings have been loaded.

Part of: #36215
Co-Authored-By: Feanil Patel <[email protected]>
UsamaSadiq pushed a commit that referenced this issue May 14, 2025
@kdmccormick @UsamaSadiq
docs: Settings Simplification ADR (#36224)
f343a0f 
Part of: #36215

Follow up in: openedx/open-edx-proposals#684
UsamaSadiq pushed a commit that referenced this issue May 14, 2025
@kdmccormick @UsamaSadiq
refactor: Clean up CMS production settings cruft (#36306)
5433ba8 
This is a pure refactoring of cms/envs/production.py, removing several
redundant statements that have accrued over the years as the platform moved
from python-only, to python+json, to python+json+yaml, to today's python+yaml
setup.

This is the CMS version of:
* a81493c
* (originally 1593923)

Also included:

* Add some more explicit structure to the both LMS's and CMS's
  production.py using big comments.

* In both LMS and CMS settings, alphabetize the production overrides,
  and remove the extraneous comments. Separate out the handful of settings
  which have useful comments. The rest of the settings' comments were not
  helpful--they were either just stating the obvious, or they were duplicative
  of what's documented in common.py.

Co-Authored-By: Feanil Patel <[email protected]>

Part of: #36215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@feanil feanil

@kdmccormick kdmccormick

Labels
code health Proactive technical investment via refactorings, removals, etc. documentation Relates to documentation improvements epic Large unit of work, consisting of multiple tasks
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@blarghmatey @feanil @kdmccormick