Revert "refactor: Clean up lms/envs/production.py cruft" - #36129
Conversation
This reverts commit 1593923.
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
Retroactive ✅ -- I agree with the revert logic and I am working with Robert right now to tease out the underlying bug.
|
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. |
|
|
||
| # This is the domain that is used to set shared cookies between various sub-domains. | ||
| # By default, it's set to the same thing as the SESSION_COOKIE_DOMAIN, but we want to make it overrideable. | ||
| SHARED_COOKIE_DOMAIN = _YAML_TOKENS.get('SHARED_kCOOKIE_DOMAIN', SESSION_COOKIE_DOMAIN) |
There was a problem hiding this comment.
This typo (thanks Ali) is the likely problem. Do we need a unit test to ensure all _YAML_TOKENS calls make it to settings?
FYI: @kdmccormick
There was a problem hiding this comment.
🤦🏻
Thanks @alangsto and @robrap .
I am open to suggestions, but I am struggling to imagine how to write unit test that would generically test what we want, since:
- We do not know what the full set of valid YAML keys is.
- Different YAML keys are handled differently. Some directly become django settings. Some are additionally used to compute defaults for other YAML keys (such as this one). Some are specially merged into common settings (like JWT_AUTH). Some are just used to derive real django settings (such as XBLOCK_EXTRA_MIXINS). Some are ignored entirely (such as BROKER_POOL_LIMIT, which is overriden back to
0after the YAML is loaded).
One thing we could do, though, is check a redacted version of 2U's production lms.yml and cms.yml into edx-platform as test data, as well as a matching lms.json and cms.json file representing the expected realized django settings. We would have a unit test to ensure that lms.yml and cms.yml yielded lms.json and cms.json, protecting against bugs like this one.
If you folks provided the redacted YAML files, I could generate the corresponding JSON files and wire up the unit test.
There was a problem hiding this comment.
It was actually a different Ali, and I'm not sure of his github username. Maybe we'll update this later, but I'll share the rest of the name privately. :)
I like your ideas, and have comments. I am going to instead comment on the new Attempt 2 PR, and will quote from here. Thanks.
UPDATE: I'm going to discuss in Slack, and we can post an update on the PR later.
4 participants
Reverts #36115
This breaks our MFEs. On edx.org, it seems the JWT cookies are getting set with the domain
.courses.stage.edx.org, making them inaccessible to<MFE>.stage.edx.org.Additionally, we had done an immediate rollback in production, and this was the only change and it fixed the broken MFEs in production.