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. DetailsWhere 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
8b509eb to
eaea6f7
Compare
|
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 🚀 |
|
Sandbox deployment successful 🚀 |
eaea6f7 to
4ba8298
Compare
|
Sandbox deployment successful 🚀 |
kdmccormick
left a comment
There was a problem hiding this comment.
Thank for you for the comprehensive testing report, new documentation, and explanatory comments. Just a few small changes, then this is good to go.
4ba8298 to
7a4cbdf
Compare
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
@wgu-taylor-payne looks like there are some conflicts that need to be resolved but then it looks like this can be merged. The sandbox failures seem un-related so hopefully we can get one running after the conflicts have been resolved. |
6d5892d to
6cef2de
Compare
|
Sandbox deployment successful 🚀 |
The issues that Kyle brought up have been resolved.
kdmccormick
left a comment
There was a problem hiding this comment.
Looks great! I smoke-tested on the PR sandbox, ensuring I could create a library, use its content in a course, publish that course, and enroll in that course.
|
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. |
Description
Pull settings explicitly shared by the LMS and CMS into a common settings module at
openedx/envs/common.pythatlms/envs/common.pyandcms/envs/common.pyinherit 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.pywill be included on the settings reference page.Supporting information
Resolves #36889.
Testing instructions
diff_settings.pyI've used the
diff_settings.shscript as a foundation to create adiff_settings.pyscript that can be found here (documentation here). I've used this script to test the rendered settings for this branch against the rendered settings in the master branch.Modification to
cms/envs/production.pyrequiredTo get all runs of
dump_settingsto work (specifically running the command withDJANGO_SETTINGS_MODULE=cms.envs.productionandCMS_CFG=cms/envs/mock.yml) I needed to make a small patch tocms/envs/production.pyin bothmasterand my branch:Without this, the command would fail and output this error:
Results from running
diff_settings.pyRunning
python diff_settings.py master tpayne/create-openedx-envs-common-settingsin a tutor dev environment, I get the following results:The
"module"lines are describing where the lambda for theJWT_AUTHsettingJWT_PAYLOAD_GET_USERNAME_HANDLERis being defined.Also, I decided to bring
USAGE_ID_PATTERN, which historically hasn't been imported in the CMS settings, intoopenedx/envs/common.pyalong withUSAGE_KEY_PATTERNandASSET_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_KEYincms/envs/common.pywhere it will contain'cms'rather than'lms'in the value, ifSERVICE_VARIANTis 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.pyto not setSERVICE_VARIANTif unset. I also had to modifydiff_settings.pyto unset theSERVICE_VARIANTbefore running thedump_settingsmanagement command. With those changes in place these differences appeared: