An initial draft of a decision around how we manage settings files. #23829
Conversation
|
|
||
| Rather than having both python and yaml override files for our dev and test | ||
| environments, we will move towards having all settings defined in a yaml file | ||
| and for all environments to use production.py to load their settings. |
There was a problem hiding this comment.
Can you add to the consequences what specific changes need to happen to support this decision?
For example, what happens to the settings in the devstack*.py files?
|
|
||
| Rather than having both python and yaml override files for our dev and test | ||
| environments, we will move towards having all settings defined in a yaml file | ||
| and for all environments to use production.py to load their settings. |
There was a problem hiding this comment.
Can you also add if there were any alternative paths considered and what the trade-offs were?
| most part this will be a wholesale replacement for any complex values(dicts, | ||
| lists, etc) but for some specific settings they will be additive for now. | ||
|
|
||
| eg. ADDITIONAL_INSTALLED_APPS, or ADDITIONAL_MIDDLEWARE |
There was a problem hiding this comment.
Can you explain why these overrides would still be needed? Why not just override through the YAML files?
| differently we will move to just a few python files with most of the settings | ||
| variance living in YAML config files that are managed by environment operators. | ||
|
|
||
| common.py - This file will house the defaults for all settings that are |
There was a problem hiding this comment.
Is this an opportunity to rename common.py to a more standard Django name (settings.py)?
There was a problem hiding this comment.
I was considering defaults.py to make it more clear as well. I'd be happy to pull this in as a part of this decision. Let me know if you prefer common.py or defaults.py
|
|
||
|
|
||
| Decision | ||
| ======== |
There was a problem hiding this comment.
Can you add a reference to the ADR/OEP for Remote config and explain how this ADR relates/adopts it?
There was a problem hiding this comment.
There was never an ADR or OEP written around remote config so I have nothing to reference.
| lists, etc) but for some specific settings they will be additive for now. | ||
|
|
||
| eg. ADDITIONAL_INSTALLED_APPS, or ADDITIONAL_MIDDLEWARE |
There was a problem hiding this comment.
I think dicts and list are different animals:
- Lists need
ADDITIONAL_settings so you can choose between adding or replacing the defaults. - Dicts are still just key/value pairs that allow someone to group settings. I would love it if updating were the rule and overwriting were the exception for dicts. But, I can see arguments against this, so I am simply stating my preference.
There was a problem hiding this comment.
I'd rather not see dict merging at all. It results in a final settings dict that may not be visible anywhere except in production making it hard to debug and understand the current state of what's set. Ansible did this and it caused more problems than it solved in my opinion. I'd rather move towards things being more explicit and providing entrypoints for adding behavior to the core.
There was a problem hiding this comment.
👍
- Sounds like you would have the same argument for lists (i.e. avoiding
ADDITIONAL_settings), as well, but not sure? - I think this is good content for the ADR, whether it is next to the decision or in rejected alternatives, or where ever. It helps clarify and explain your decision, even if cleaning up certain exceptions doesn't come until later.
|
|
||
| Rather than having both python and yaml override files for our dev and test | ||
| environments, we will move towards having all settings defined in a yaml file | ||
| and for all environments to use production.py to load their settings. |
There was a problem hiding this comment.
@nasthagiri had also mentioned the other day that there may be a consequence for Open edX that currently relies on environment variables. This isn't just about moving things around in files, right? Environment variables won't work like they used to?
There was a problem hiding this comment.
I do believe that any changes in settings/configuration could potentially impact Open edX - since that's one of their primary bread-n-butter usage of the platform.
To minimize multiple DEPR announcements, should we include the changes proposed in this ADR with the changes we're proposing for replacing (Django Switches, Configuration Models, Site Configuration) with (Django Settings) in a single DEPR ticket?
There was a problem hiding this comment.
The changes here so far have not broken any compatibilty requirements, and I don't expect them to. In general we are moving from settings being implied to being explicit but where we need to pull settings from the environment, we can add that as a capability to production.py. I'm not familiar of any use-cases for this in edx-platform right now except for some of the work done around the static asset CDN.
|
This seems to describe a different scheme than OEP-45 describes, or maybe I am misunderstanding? |
|
@nedbat Good point. I just read and reviewed that OEP. We'll need to reconcile with the proposed config-related changes there. In many ways, this is a great "sister" ADR to go along with that OEP - to move forward these changes together. |
|
|
||
|
|
||
| Decision | ||
| ======== |
There was a problem hiding this comment.
There was never an ADR or OEP written around remote config so I have nothing to reference.
| lists, etc) but for some specific settings they will be additive for now. | ||
|
|
||
| eg. ADDITIONAL_INSTALLED_APPS, or ADDITIONAL_MIDDLEWARE |
There was a problem hiding this comment.
I'd rather not see dict merging at all. It results in a final settings dict that may not be visible anywhere except in production making it hard to debug and understand the current state of what's set. Ansible did this and it caused more problems than it solved in my opinion. I'd rather move towards things being more explicit and providing entrypoints for adding behavior to the core.
|
|
||
| Rather than having both python and yaml override files for our dev and test | ||
| environments, we will move towards having all settings defined in a yaml file | ||
| and for all environments to use production.py to load their settings. |
There was a problem hiding this comment.
The changes here so far have not broken any compatibilty requirements, and I don't expect them to. In general we are moving from settings being implied to being explicit but where we need to pull settings from the environment, we can add that as a capability to production.py. I'm not familiar of any use-cases for this in edx-platform right now except for some of the work done around the static asset CDN.
|
@nasthagiri @robrap I updated the decision here to better match up with what's in OEP-45 which I think makes a fine enough suggestion that we don't need to mess with it for edx-platform other than to handle LMS vs CMS. |
feanil
force-pushed
the
feanil/settings_adr
branch
from
c2d49e8 to
c0f28a7
Compare
|
Your PR has finished running tests. There were no failures. |
| Context | ||
| ======= | ||
|
|
||
| Settings are confusing right now and there are way too many layers. We have |
There was a problem hiding this comment.
Nit: we had decided to use auto-wrap rather than having fixed line lengths in our doc files.
There was a problem hiding this comment.
I'll fix this before merging, I wrapping for review because it makes the diffs a bit more readable as I make changes. Also allows for better pinpointing of where to make the change when that's relevant.
|
|
||
| ``defaults.py`` will house and document all default settings. | ||
|
|
||
| ``required.py`` will document and validate that required settings have been defined. |
There was a problem hiding this comment.
Will the validation of the required settings happen within required.py itself or in a separate module that gets called during the Django startup phase? My read of OEP-45 is that non-defaulted required settings are declared in required.py, not necessarily validated in this file.
There was a problem hiding this comment.
The application will check that operators provided these values, and will not start unless they are set. - This is from the description of required.py which I think is not very specific about where this should be but I think it makes sense to put this validation in required.py. Though I think I see an issue with this that I'll ask about on the OEP-45 PR.
| the public repository. | ||
| * This wouldn't solve the problem where we would still try to "inherit" from other settings files and make it harder to read the current value of any given setting. | ||
|
|
||
| This alternative gives us a lot more power but it's power that we don't actually need. Building limitiations into what settings can be helps us keep them simple and understandable. |
There was a problem hiding this comment.
limitiations -> limitations
| * test.py | ||
| * test_static_optimized.py | ||
|
|
||
| If there exist default YAML files for any of the above environments they should |
There was a problem hiding this comment.
Does it make sense for this ADR to also list which YAML config files we expect to manage within edx-platform? And these YAML files would be within the same env folder as defaults.py, right?
There was a problem hiding this comment.
I'd rather go try it out and then come back with the list of yaml file if it makes sense. Some of the yaml files might live in this folder but others might not. For example the sandbox yaml file might live in our sandbox config repo because it still has some secrets in it that we wouldn't want to put in the public repo.
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage |
|
EdX Release Notice: This PR has been deployed to the production environment. |
No description provided.