MFE dynamic config #69
Conversation
There was a problem hiding this comment.
Thanks for continuing work on this @ghassanmas . I have a few concerns that I think can be addressed by things I learned when working on this myself.
|
Cancel that. It's an unrelated issue. |
ghassanmas
force-pushed
the
mfe-dynamic-config
branch
4 times, most recently
from
7fea02a to
a7e1941
Compare
|
FYI I'm not closely monitoring this PR. It seems that you three are having very a productive conversation and I trust you to reach the right decisions. Feel free to @ me at any point, though. |
ghassanmas
force-pushed
the
mfe-dynamic-config
branch
2 times, most recently
from
8e6eb70 to
2bff31b
Compare
There was a problem hiding this comment.
We're getting there, @ghassanmas! Thanks for the changes so far. I made some additional comments.
|
@ghassanmas, can you also rebase on master, please? |
|
@arbrandes you mean rebsing the PR or the branch? |
|
@ghassanmas, I mean your |
At this point, with Olive being so close to release, I think it's fine if we merge either in the Olive or the nightly branch. In Olive you'll be able to By the way, I'm guessing that you have to frequently merge nightly in olive, and then rebase your changes on top of it. I'd like to attract your attention to the (this is what I'm doing with the olive branch of tutor core) |
At least I'm not. I'm intentionally forgetting the olive branch exists, and instead focusing on nightly. When we're sure of the feature-set we want for Olive (including which MFEs) and we're on top of the release, I'll pull nightly into olive and do whatever else the upgrade requires.
I did not know |
ghassanmas
force-pushed
the
mfe-dynamic-config
branch
from
de3c761 to
c1d364c
Compare
| @@ -14,3 +14,31 @@ PROFILE_MICROFRONTEND_URL = "{% if ENABLE_HTTPS %}https://{% else %}http://{% en | |||
| LOGIN_REDIRECT_WHITELIST.append("{{ MFE_HOST }}") | |||
| CORS_ORIGIN_WHITELIST.append("{% if ENABLE_HTTPS %}https://{% else %}http://{% endif %}{{ MFE_HOST }}") | |||
| CSRF_TRUSTED_ORIGINS.append("{{ MFE_HOST }}") | |||
|
|
|||
| ALLOWED_HOSTS.append("{{ MFE_HOST }}") | |||
There was a problem hiding this comment.
Why is this needed now if it wasn't needed before?
There was a problem hiding this comment.
I don't remember the exact reason, may be because now LMS would recevice a request from MFE_HOST as a proxy, might not be necessary but then would need to double test again
There was a problem hiding this comment.
OK, I looked into it a bit more. From https://docs.djangoproject.com/en/4.1/ref/settings/#allowed-hosts:
ALLOWED_HOSTS
A list of strings representing the host/domain names that this Django site can serve.
Given the new proxying, it make sense that we need this 👍🏻
Please just add a brief comment to this line explaining that we need it so that the LMS can serve API requests with MFE_HOST as a proxy.
There was a problem hiding this comment.
I'm having a second look at this. I think that the "right" way to do this is not to modify ALLOWED_HOSTS, which is a sensitive setting, but to modify the Caddy configuration to automatically modify the "Host" header on forwarding to the LMS. I'll open a PR.
EDIT: here is the PR https://github.com/overhangio/tutor-mfe/pull/97/files
There was a problem hiding this comment.
Yes it can rewrite the headers, I was able to remove that line while changing the caddy patch to
reverse_proxy /api/mfe_config/v1* lms:8000 {
header_up Host {http.reverse_proxy.upstream.hostport}
}That did it for me.
This change does a couple of things and it introduce breaking
changes, in summary it does:
- It remove the usage of env/(productoin/development)
- It's no longer possible to set/override MFE config via
MFE_APP_*
- It make MFEs rely to config that are defied in LMS
settins
- Other essetinal config that are needed at build time are
are defined in the Dockerfile
- It sets a proxy for both Caddy (for production) and Webpack
(for development)
- For old patches that are used to set/override MFE config, via
production/development would need to be rewritten
ghassanmas
force-pushed
the
mfe-dynamic-config
branch
from
c1d364c to
9412051
Compare
|
@arbrandes @ghassanmas have either of you found this to work in dev mode? I am finding:
|
|
@kdmccormick, I have yet to test dev mode in any depth, but it sounds to me like those could be upstream issues. In any case, I'll actually test and report back. |
(totally a sidenote) I agree with you @arbrandes. When I proposed the merging model for the nightly branch, it was really for the lack of any better option with rebase. If you have any better idea then I'm very much open to suggestions. (but let's discuss this after this PR is merged 😄) |
This change make it possible if LMS url to be changed, that the last value will be picked. This is simlair openedx/frontend-app-authoring/pull/389 which issue overhangio/tutor-mfe/issues/86, the fixes is needed so that dynamic config would work with tutor: overhangio/tutor-mfe/pull/69
|
Here are the results of my testing. tutor localI've tested this via tutor devAs for
I could not reproduce this:
I am seeing a console warning, but that doesn't seem to affect the functioning of the MFE at all. (I'd venture a guess that this would show in the devstack as well, though I haven't tried reproducing it there, yet.) Warning: Failed prop type: The prop `id` is marked as required in `ForwardRef(DropdownToggle)`, but its value is `undefined`.
Could not reproduce this, either. Maybe if you can provide more details on your environment? Was it nightly? Did you just ConclusionIf we agree to not wait for 2U and instead review/test/merge the Gradebook PR ourselves, I think we can merge this. |
nightly, Unfortunately there are so many variables at work here that I really cannot tell whether this was a one-off thing with my environment or a real problem with the PR. If it worked for you, then it's more likely my environment. Thing is, this PR is a release-blocker anyway. If it turns out there is an bug in it, we'll need to delay the release, whether or not it's merged. So, let's just merge, which will allow more people to test it between now and Monday, thus giving us more data on whether it's my environment or an implementation problem. |
|
Huge thanks @ghassanmas (and @kdmccormick and @regisb), and congratulations! |
|
This is an amazing achievement, huge kudos to you three. |
This change make it possible if LMS url to be changed, that the last value will be picked. This is simlair openedx/frontend-app-authoring/pull/389 which issue overhangio/tutor-mfe/issues/86, the fixes is needed so that dynamic config would work with tutor: overhangio/tutor-mfe/pull/69
This change make it possible if LMS url to be changed, that the last value will be picked. This is simlair openedx/frontend-app-authoring/pull/389 which issue overhangio/tutor-mfe/issues/86, the fixes is needed so that dynamic config would work with tutor: overhangio/tutor-mfe/pull/69
When introducing dynamic config[1], `CSRF_TOKEN_API_PATH` was removed from the MFE build environment and not added to the corresponding Django settings, leading to POST failures across the board. This addresses the problem by adding `CSRF_TOKEN_API_PATH` back in. [1] overhangio#69
When introducing dynamic config[1], `CSRF_TOKEN_API_PATH` was removed from the MFE build environment and not added to the corresponding Django settings, leading to POST failures across the board. This addresses the problem by adding `CSRF_TOKEN_API_PATH` back in. [1] #69
When introducing dynamic config[1], `CSRF_TOKEN_API_PATH` was removed from the MFE build environment and not added to the corresponding Django settings, leading to POST failures across the board. This addresses the problem by adding `CSRF_TOKEN_API_PATH` back in. [1] #69
When introducing dynamic config[1], `CSRF_TOKEN_API_PATH` was removed from the MFE build environment and not added to the corresponding Django settings, leading to POST failures across the board. This addresses the problem by adding `CSRF_TOKEN_API_PATH` back in. [1] overhangio#69 (cherry picked from commit 413ed4d)
Related to kdmccormick/pull/1
When building the docker image each MFE was built from the its crossponding PR:
The built image is pushed at https://hub.docker.com/r/16084536/mfe
How to test/USE
tutorfrom feat: Upgrade to Olive tutor#734tutor-mfefrom this PR/branchdocker pull 16084536/mfe:olive.runtime.amd.1tutor config save --set MFE_DOCKER_IMAGE=docker.io/16084536/mfe:olive.runtime.amd.1Refs
Dockerfile (How did the Dockerfile looked like prior to build?)
Dockerfile source
env/production
env/production
Caddyfile
Caddyfile