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 mfe config api #30473

Merged
merged 10 commits into from
Jul 8, 2022
Merged

Conversation

MaferMazu
Copy link
Contributor

@MaferMazu MaferMazu commented May 24, 2022

Description

This PR adds MFE Config API. This is part of the work that is being done to obtain the MFE Runtime Configurations and that has been discussed in the BTR WG.

Testing instructions

  1. Use this edx-platform version in you tutor installation
  2. Enable the API
    2.1. In the tutor_root (to know your tutor_root: tutor config printroot ), in the env > apps > openedx > config > lms.env.json file add "ENABLE_MFE_CONFIG_API": true,
  3. Run the tutor in dev mode tutor dev start
  4. You can make request to the API (endpoint /api/v1/mfe_config)
    4.1. Example: <your_domain>/api/v1/mfe_config
    4.2 Example: <your_domain>/api/v1/mfe_config?mfe=mymfe

This API takes the information found in MFE_CONFIG and MFE_CONFIG_MYMFE key in site configuration ( admin > site configuration > your domain); and if you want to add information you just have to add something like this:

"MFE_CONFIG" : {
    "LOGO_URL": "https://example.com/logo.png",
},
"MFE_CONFIG_MYMFE" : {
    "LOGO_URL": "https://example.com/mymfe-logo.png",
}

@openedx-webhooks
Copy link

openedx-webhooks commented May 24, 2022

Thanks for the pull request, @MaferMazu! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed open-source-contribution PR author is not from Axim or 2U labels May 24, 2022
@natabene
Copy link
Contributor

@MaferMazu Thank you for your contribution. Please let me know once it is ready.

@arbrandes arbrandes self-requested a review May 24, 2022 20:13
@arbrandes
Copy link
Contributor

Thanks for this, @MaferMazu! It's on my queue to review ASAP.

@MaferMazu
Copy link
Contributor Author

Thanks @arbrandes
By the way, I don't know if openedx/core/djangoapps is the best place to add the mfe api or if it should be in lms/djangoapps, what do you think?

@MaferMazu MaferMazu force-pushed the mfmz/mfe-config-api branch 2 times, most recently from f56c223 to 0aebf4b Compare May 26, 2022 22:50
lms/envs/common.py Outdated Show resolved Hide resolved
@MaferMazu MaferMazu force-pushed the mfmz/mfe-config-api branch 6 times, most recently from 6775936 to 6f30f77 Compare May 30, 2022 18:10
@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented May 30, 2022

I'm not sure if this is the final location for this application, either way, please consider this ADR for new applications that's underway.

@MaferMazu
Copy link
Contributor Author

Thanks a lot for that info @mariajgrimaldi I am going to take a look and make a decision about the location for this app.

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

The API looks very lightweight and I love that. I left a note inline, but once that is fixed (or discussed at least) I would approve.

lms/urls.py Outdated Show resolved Hide resolved
@felipemontoya
Copy link
Member

We could remove the draft label though.

@MaferMazu MaferMazu marked this pull request as ready for review May 31, 2022 16:17
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 31, 2022
@MaferMazu
Copy link
Contributor Author

MaferMazu commented May 31, 2022

Thanks @mariajgrimaldi for mentioning the ADR for new applications, however, this PR allows configuration in runtime with mfes and we would like to solve this problem in the community, for this reason, I still want to include this little API.

I am also super attentive to any comments.

@edx-pipeline-bot
Copy link
Contributor

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

@arbrandes
Copy link
Contributor

@MaferMazu, congrats on the merge, and thank you for your patience!

@edx-pipeline-bot
Copy link
Contributor

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

@kdmccormick
Copy link
Member

Thanks for all your work on this! It is going to help us provide a reasonably-sized, pre-built Tutor MFE image, which is something that folks have wanted for a long time.

If it's not too late, I have a couple suggested changes:

  • Document the behavior of the API response in the docstring.
    • Without the query parameter, only common config is returned; with the query paramter, MFE's specific config is merged with common config. This design is great! The API's docstring doesn't explain it, though--I had to read the code and the ADR to figure it out.
  • Change the base URL from /api/v1/mfe_config to /api/mfe_config/v1.
    • Why? Because the version number (v1) should come after the thing it is versioning. In the current construction, it is implying that mfe_config is part of the first version of the entire LMS's API. In reality, it is just the first version of the mfe_config API.

@kdmccormick
Copy link
Member

Hey @MaferMazu and @felipemontoya, I've proposed some changes (including the ones I mentioned above) to the API: #30830

Would either of you mind taking a look?

ztraboo added a commit to CUCWD/edx-platform that referenced this pull request Oct 26, 2022
navinkarkera pushed a commit to open-craft/edx-platform that referenced this pull request Mar 23, 2023
feat: add mfe config api
(cherry picked from commit 0bb4577)
johanseto pushed a commit to nelc/edx-platform that referenced this pull request Jan 22, 2024
becdavid added a commit to CUCWD/edx-platform that referenced this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.