Skip to content
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

feat: add api MFE configuration #745

Merged
merged 4 commits into from
Jun 17, 2023

Conversation

dcoa
Copy link

@dcoa dcoa commented May 1, 2023

feat: add api MFE configuration

Description

This PR is a backport of all the commits related to MFE API configuration.

Supporting information

The related PR are:

Testing instructions

Please, follow the instructions here openedx/edx-platform#30473

Additional information

JIRA CARD

felipemontoya and others added 4 commits May 2, 2023 06:06
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.
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".
* 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 visited
  https://courses.edx.org/api-docs, you could see that the API was
  listed under "v1" instead of "mfe_config".
@dcoa dcoa requested review from MaferMazu, Alec4r and luisfelipec95 May 1, 2023 20:20
@MaferMazu
Copy link
Contributor

Hello Diana, thanks for this, but I wonder if we want this PR. The acceptance criteria for that Jira card are more about having a "productive" branch about CSS variables in the frontend platform and learning mfe and having a strain published around your work of CSS variables.

@santiagosuarezedunext @Alec4r, do you know if someone on the team uses MFE in Nutmeg and probably wants this?

The backport is okay, and these changes won't break things, but as I said, I am not so sure we want this because, as eduNEXT, we are trying these things in Olmo.

@dcoa
Copy link
Author

dcoa commented May 9, 2023

@MaferMazu let me explain a little bit more the context:

  1. Point 1 of the acceptance criteria talks about nuez and olmo releases.
  2. The third acceptance criteria requests to describe how to have an environment with CSS variables, which is why we need this backport.

@dcoa
Copy link
Author

dcoa commented May 10, 2023

Understanding that we are gonna use this in Olmo, I proceed to close this PR.

@dcoa dcoa closed this May 10, 2023
@dcoa dcoa reopened this Jun 8, 2023
@dcoa dcoa changed the base branch from ednx-release/nuez.master to ednx-release/nuez.runtime-mfe June 8, 2023 23:16
@dcoa dcoa requested a review from JuanDavidBuitrago June 8, 2023 23:18
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Copy link
Member

@JuanDavidBuitrago JuanDavidBuitrago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, but I wonder how can we switch between have or no have MFE configurations, as we know, MFE is not mandatory in Nutmeg/Nuez

If this is not a problem, go ahead!

@dcoa
Copy link
Author

dcoa commented Jun 13, 2023

It's all about configurations to enable the API you should have ENABLE_MFE_CONFIG_API in true and disable or enable MFE's with the django flags.

@JuanDavidBuitrago
Copy link
Member

Thanks for the clarification @dcoa.

@dcoa dcoa changed the base branch from ednx-release/nuez.runtime-mfe to ednx-release/nuez.master June 16, 2023 01:00
@dcoa dcoa merged commit a91a5a8 into ednx-release/nuez.master Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants