Suggested changes to the MFE Config API

[kdmccormick]

Description

This PR contains three suggested changes to the MFE Config API , which was recently added to the LMS. The changes are broken into separate commits. ⚠️ Changes 1 and 3 are backwards-incompatible. ⚠️

1. feat!: change names of dynamic MFE config Django settings
   • Formerly, the settings were:
     • MFE_CONFIG for common config.
     • MFE_CONFIG_<APP_ID> for app-specific overrides, with each app getting its own Django setting.
   • This commit changes it to:
     • MFE_CONFIG for common config (unchanged).
     • MFE_CONFIG_OVERRIDES for app-specific overrides, where each app gets a top-level key in the dictionary.
   • Why the change?
     • We want common.py to have a complete list of overridable settings, as it helps operators reason about configuration and allows us to generate config documentation using toggle annotations. Dynamically generating setting names based on arbitrary APP_IDs makes this impossible.
     • getattr(...) generally makes code more complicated bug prone. Tools like pylint and mypy cannot effectively analyze any code that uses dynamic attribute access.
2. docs: add more detail to MFE Config API documentation
   • No functional changes here. This just uses the edx_api_doc_tools package to add some additional documentation to the new API. The documentation can be read from the code, or viewed by visiting http://<LMS_ROOT>/api-docs and searching for "mfe_config":
     
3. feat!: change /api/v1/mfe_config to /api/mfe_config/v1
   • This changes the API's path. The reasoning is that this is Version 1 of the mfe_config API, not Version 1 of the LMS's entire API, so the v1 should come after mfe_config.
   • Why does this matter? Firstly, consistency. Secondly, it affects our generated API documentation. If you visit https://courses.edx.org/api-docs, you can see that the API is currently listed under "v1" instead of "mfe_config":
     
   • This PR would change that:
     

Deadline

Sooner is better than later because it seems like the MFE Config API will be used in some production instances very soon, if it's not already.

[felipemontoya]

Thanks @kdmccormick

I think the 3 modifications you are proposing are reasonable and would make this API better.
The implementation seems OK although I did not fire up my stack to try it.

I would ask @MaferMazu for a quick review since she was the original author. Mafer, the sooner you can schedule this the better because we would rather not have anyone using the current version in prod if we are going to add breaking changes.

Also fyi @dcoa, have this change in mind. I believe only the URL change would affect your work, but still.

[felipemontoya]

@arbrandes maybe you would also sign off on this?

[MaferMazu]

Thanks for tagging me @felipemontoya . I'm going to take a look at it.

[MaferMazu]

@kdmccormick Thanks for the suggested changes; I agree with most. I just wanted to ask about the benefits of using settings.MFE_CONFIG over getattr(settings, "MFE_CONFIG", {}). I think the second way is safer since it specifies a default if it doesn't find the MFE_CONFIG; if you can share documentation of the problems you mention in the PR would appreciate it.

[kdmccormick]

@MaferMazu settings.MFE_CONFIG is safe because MFE_CONFIG = {} is defined in lms/envs/common.py, which means it will always be defined on LMS's settings object.

In fact, settings.MFE_CONFIG is safer, because it will raise an error if:

• one tries to reference settings.MFE_CONFIG in CMS (Studio)
• one spells the setting name wrong, like settings.MFE_CFG

If someone wrote, for example, getattr(settings, "MFE_CFG", {}), it would fail silently. They would not see a pylint error nor a runtime error, so it's more likely the bug would go unnoticed. This is why getattr(...) is something that should be used sparingly.

[kdmccormick]

Thanks for the review @MaferMazu !

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.