-
Notifications
You must be signed in to change notification settings - Fork 4.1k
create openedx envs common settings #36941
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
base: master
Are you sure you want to change the base?
create openedx envs common settings #36941
Conversation
Thanks for the pull request, @wgu-taylor-payne! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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. 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. |
48c86a1
to
9c610f5
Compare
|
||
from openedx.core.constants import COURSE_KEY_REGEX, COURSE_KEY_PATTERN, COURSE_ID_PATTERN | ||
from openedx.envs.common import * # pylint: disable=wildcard-import | ||
|
||
from lms.envs.common import ( |
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.
I didn't move these up, since it is unclear whether they belong in the CMS long term.
8b509eb
to
eaea6f7
Compare
To create section headers in this file, use the following function: | ||
|
||
```python | ||
def center_with_hashes(text: str, width: int = 76): | ||
print(f"{f' {text} ':#^{width}}") | ||
``` |
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.
Not sure if this should be included, but if we decide to continue marking sections in this way, it is nice to be able to create them in a standardized style.
MEDIA_URL = '/media/' | ||
|
||
# Dummy secret key for dev/test | ||
SECRET_KEY = 'dev key' |
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.
This is a variable which wasn't explicitly imported into the cms settings from the lms settings, but it is used by such a setting (JWT_AUTH
).
|
||
DEBUG_TOOLBAR_PATCH_SETTINGS = False | ||
|
||
################ Shared Functions for Derived Configuration ################ |
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.
I have decided to improve the docstrings for the two functions in this section. Also, since they are declared here and then imported into the lms and cms settings modules, I have removed the leading _
in their names which had marked them as private.
Doing this also makes them available if we use from openedx.envs.common import *
to import the things defined in this file into downstream settings files.
# .. setting_name: PARENTAL_CONSENT_AGE_LIMIT | ||
# .. setting_default: 13 | ||
# .. setting_description: The age at which a learner no longer requires parental consent, | ||
# or ``None`` if parental consent is never required. | ||
PARENTAL_CONSENT_AGE_LIMIT = 13 | ||
|
||
############################### Registration ############################### | ||
|
||
# .. setting_name: REGISTRATION_EMAIL_PATTERNS_ALLOWED | ||
# .. setting_default: None | ||
# .. setting_description: Optional setting to restrict registration / account creation | ||
# to only emails that match a regex in this list. Set to ``None`` to allow any email (default). | ||
REGISTRATION_EMAIL_PATTERNS_ALLOWED = None |
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.
This documentation did not exist, but I added it to make sure that such documentation declared in this module would appear in the settings reference docs page.
'small': 30 | ||
} | ||
|
||
############################## Miscellaneous ############################### |
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.
I wasn't readily sure how to categorize the settings added here.
# They are used so that URLs with deprecated-format strings still work. | ||
USAGE_KEY_PATTERN = r'(?P<usage_key_string>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))' | ||
ASSET_KEY_PATTERN = r'(?P<asset_key_string>(?:/?c4x(:/)?/[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))' | ||
USAGE_ID_PATTERN = r'(?P<usage_id>(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+))' |
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.
USAGE_KEY_PATTERN
and ASSET_KEY_PATTERN
were being imported into the cms settings from the lms settings, but USAGE_ID_PATTERN
was not. I decided to add USAGE_ID_PATTERN
here since it is closely related to the other two settings. If we don't care as much about the co-location of these settings, then we can move USAGE_ID_PATTERN
back into lms/envs/common.py
.
@@ -6,6 +6,12 @@ This is the list of (non-toggle) Django settings defined in the ``common.py`` mo | |||
.. note:: | |||
Toggle settings, which enable or disable a specific feature, are documented in the :ref:`feature toggles <featuretoggles>` section. | |||
|
|||
Open edX settings |
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.
Is this a good section title for the settings reference page?
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.
Let's call it "Platform-Wide Settings". "Open edX" includes things outside of edx-platform (and the openedx/ directory is poorly named).
@@ -1539,6 +1458,10 @@ | |||
CELERY_BROKER_USE_SSL = False | |||
CELERY_EVENT_QUEUE_TTL = None | |||
|
|||
############################## HEARTBEAT ###################################### | |||
|
|||
HEARTBEAT_CELERY_ROUTING_KEY = HIGH_PRIORITY_QUEUE |
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.
HEARTBEAT_CELERY_ROUTING_KEY
has previously been imported from lms/envs/common.py
, but when I was looking into it, that LMS setting can/will contain lms
in this str
in the event that SERVICE_VARIANT
is not set (I'm not sure how rare this scenario would be).
These variables follow a similar pattern in both lms/envs/common.py
and cms/envs/common.py
. Here is how HEARTBEAT_CELERY_ROUTING_KEY
gets its value in lms/envs/common.py
:
SERVICE_VARIANT = os.environ.get('SERVICE_VARIANT', 'lms') # 'cms' in cms/envs/common.py
CONFIG_PREFIX = SERVICE_VARIANT + "." if SERVICE_VARIANT else ""
QUEUE_VARIANT = CONFIG_PREFIX.lower()
HIGH_PRIORITY_QUEUE = f'edx.{QUEUE_VARIANT}core.high'
HEARTBEAT_CELERY_ROUTING_KEY = HIGH_PRIORITY_QUEUE
So, cms/envs/common.py
has the same pattern (except it defaults to 'cms'
for the SERVICE_VARIANT
), but then it imports HEARTBEAT_CELERY_ROUTING_KEY
from the lms settings rather than using HIGH_PRIORITY_QUEUE
as the value like in lms/envs/common.py
.
Is that how we should keep it, or should the HEARTBEAT_CELERY_ROUTING_KEY
value in the cms settings contain 'cms'
rather than 'lms'
in the key when/if the SERVICE_VARIANT
is unset?
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.
Another thing to add here is I can't find anywhere the cms uses HEARTBEAT_CELERY_ROUTING_KEY
.
I have left in |
Excellent work. I'm busy for the next couple days with conference preparation, so I'll see if any other CCs are available to review. If not, I'll review as soon as I'm able to. |
Sandbox deployment failed 💥 |
Sandbox deployment successful 🚀 |
Description
Pull settings explicitly shared by the LMS and CMS into a common settings module at
openedx/envs/common.py
thatlms/envs/common.py
andcms/envs/common.py
inherit from. For now, only settings explicitly shared settings (via import incms/envs/common.py
) were targeted to move up into the new shared module.This is one of the initial steps in the effort to simplify Django settings in the edx-platform (see the related ADR: 0022-settings-simplification).
Documented settings in
openedx/envs/common.py
will be included on the settings reference page.Supporting information
Resolves #36889.
Testing instructions
I've used the
diff_settings.sh
script as a foundation to create thisdiff_settings.py
script that I used to test the settings changes.diff_settings.py
Modifications required
To get all runs of
dump_settings
to work (specifically running the command withDJANGO_SETTINGS_MODULE=cms.envs.production
andCMS_CFG=cms/envs/mock.yml
) I needed to make a small patch tocms/envs/production.py
in bothmaster
and my branch:Without this, the command would fail and output this error:
Results from running diff_settings.py
Running
python diff_settings.py master tpayne/create-openedx-envs-common-settings
in a tutor dev environment, I get the following results:The
"module"
lines are describing where the lambda for theJWT_AUTH
settingJWT_PAYLOAD_GET_USERNAME_HANDLER
is being defined.Also, I decided to bring
USAGE_ID_PATTERN
, which historically hasn't been imported in the CMS settings, intoopenedx/envs/common.py
along withUSAGE_KEY_PATTERN
andASSET_KEY_PATTERN
, which have been imported into the CMS settings. These settings are similar and I thought it would make sense to declare them in the same place.I also made a modification to
HEARTBEAT_CELERY_ROUTING_KEY
incms/envs/common.py
where it will contain'cms'
rather than'lms'
in the value, ifSERVICE_VARIANT
is not set (I don't know if this would be a common scenario or not). To test the difference with this setting I had to modifymanage.py
to not setSERVICE_VARIANT
if unset. I also had to modifydiff_settings.py
to unset theSERVICE_VARIANT
before running thedump_settings
management command. With those changes in place these differences appeared:dump_settings invocation information
The code in
diff_settings.py
will run commands like these for both branches (replacing{branch_dump_dir}
depending on which branch is checked out):And here is a table of the variables involved: