Skip to content
This repository was archived by the owner on Aug 1, 2024. It is now read-only.

feat: add frontend app authn#894

Merged
zainab-amir merged 1 commit intomasterfrom
zamir/VAN-315/add_frontend_app_authn
Jan 31, 2022
Merged

feat: add frontend app authn#894
zainab-amir merged 1 commit intomasterfrom
zamir/VAN-315/add_frontend_app_authn

Conversation

@zainab-amir
Copy link
Copy Markdown
Contributor

@zainab-amir zainab-amir commented Jan 27, 2022

Ticket: https://openedx.atlassian.net/browse/VAN-315


I've completed each of the following or determined they are not applicable:

  • Made a plan to communicate any major developer interface changes (or N/A)

@zainab-amir zainab-amir force-pushed the zamir/VAN-315/add_frontend_app_authn branch from 6ce2320 to 1a13d17 Compare January 27, 2022 06:16
@zainab-amir zainab-amir force-pushed the zamir/VAN-315/add_frontend_app_authn branch from 1a13d17 to 5ca807b Compare January 28, 2022 11:35
@zainab-amir zainab-amir force-pushed the zamir/VAN-315/add_frontend_app_authn branch from 5ca807b to 139fca1 Compare January 31, 2022 06:09
@zainab-amir zainab-amir merged commit b5e428f into master Jan 31, 2022
@zainab-amir zainab-amir deleted the zamir/VAN-315/add_frontend_app_authn branch January 31, 2022 06:37
@kdmccormick
Copy link
Copy Markdown
Contributor

kdmccormick commented Feb 3, 2022

@zainab-amir you asked in Slack:

Does this change need to be reverted?
Asking because @robrap wanted me to confirm this from you.
ADR: https://github.com/openedx/devstack/blob/master/docs/decisions/0004-backends-depend-on-frontends.rst

First of all: No, I do not think the entire change needs to be reverted. It's good that the Authn MFE is available in devstack!

The concern we found with the ADR you linked (described under "status: reverted") is that it would be unsustainable if starting the LMS also started dozens of MFEs automatically because of the high resource usage of each MFE's container. So, for the majority of MFEs, we decided that the LMS should not start them by default; instead, it's up to devstack users to start all the MFEs that they are going to use. By that logic, I would recommend that you keep the majority of this PR, but revert the part that makes LMS require Authn MFE, instead leaving it the MFE as an optional component.

On the other hand, the Authn MFE is special because the login+registration flow is a gateway to almost every other feature of the platform... so, if the LMS were going to require a single MFE, it would be the Authn MFE. By that logic, it could be fine to leave this PR in place entirely.

I do not have a strong opinion either way. If I had to give a personal recommendation, I would say just leave it as-is and reevaluate if it ever becomes a problem. Our current plan is to replace Devstack with Tutor, so hopefully you won't have to worry about this for long.

@robrap
Copy link
Copy Markdown
Contributor

robrap commented Feb 3, 2022

I like @kdmccormick’s recommendation of making this an exception unless and until it becomes a problem. Two possible follow-ups:

  1. Note in the ADR, or add a new ADR, that although the original ADR remains rejected in general, an exception is being made for authn.
  2. Send a communication about all of this so others do not think it is ok to add all ne MFEs by default. Noting that it is still ok to add the MFEs, if that was confusing.

@waheedahmed
Copy link
Copy Markdown
Contributor

Thank you for the thoughts @kdmccormick and @robrap!

@zainab-amir I guess let's remove the Authn MFE from LMS dependency for now since it's not quite yet ready for the community until we complete the optional fields work. Once that is done, maybe we can then add it back and announce it community-wide.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants