fix: Dynamic port for LEARNING MFE in form of ENV variable#164
fix: Dynamic port for LEARNING MFE in form of ENV variable#164ktyagiapphelix2u merged 2 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces dynamic port configuration for the LEARNING MFE (Micro Frontend) by replacing hardcoded port 2000 with environment variables that default to port 2010.
- Updates environment variable references across Python configuration files and Docker Compose
- Adds script to dynamically update
.env.developmentfiles for all frontend applications - Modifies devcontainer configuration to use the new port and environment variables
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| py_configuration_files/lms.py | Updates CORS origins and microfrontend URL to use environment variable |
| py_configuration_files/enterprise_subsidy.py | Changes frontend app learning URL to use environment variable |
| py_configuration_files/ecommerce.py | Updates CORS origins with environment variable for learning MFE |
| py_configuration_files/course_discovery.py | Updates CORS allowed origins to use environment variable |
| docker-compose.yml | Makes port mapping dynamic using environment variables |
| .devcontainer/updateContentCommand.sh | Adds script to update MFE .env.development files with new port configuration |
| .devcontainer/devcontainer.json | Updates container environment and port forwarding configuration |
Comments suppressed due to low confidence (1)
py_configuration_files/lms.py
Outdated
| 'localhost:18400', # frontend-app-publisher | ||
| 'localhost:1993', # frontend-app-ora-grading | ||
| 'localhost:1996', # frontend-app-learner-dashboard | ||
| LEARNING_MICROFRONTEND_NETLOC, # frontend-app-learning |
There was a problem hiding this comment.
The variable LEARNING_MICROFRONTEND_NETLOC is used but not defined in this file. This will cause a NameError when the code runs.
| ENTERPRISE_CATALOG_URL = 'http://edx.devstack.enterprise-catalog:18160' | ||
| ENTERPRISE_SUBSIDY_URL = 'http://localhost:18280' | ||
| FRONTEND_APP_LEARNING_URL = 'http://localhost:2000' | ||
| FRONTEND_APP_LEARNING_URL = os.environ.get('LEARNING_MICROFRONTEND_URL', 'http://localhost:2010') |
There was a problem hiding this comment.
The os module is used but not imported. Add 'import os' at the top of the file to avoid a NameError.
| echo "Updating .env.development files for MFE apps..." | ||
|
|
||
| # Define shared values | ||
| PORT=${LEARNING_MICROFRONTEND_PORT:-2000} |
There was a problem hiding this comment.
The default port value 2000 is inconsistent with the new default port 2010 used elsewhere in the codebase. This should be 2010 for consistency.
| PORT=${LEARNING_MICROFRONTEND_PORT:-2000} | |
| PORT=${LEARNING_MICROFRONTEND_PORT:-2010} |
| PORT=${LEARNING_MICROFRONTEND_PORT:-2000} | ||
| BASE_URL=${LEARNING_MICROFRONTEND_URL:-"http://localhost:$PORT"} | ||
|
|
||
| # Path to workspace |
There was a problem hiding this comment.
This comment appears incomplete and doesn't describe any actual code. It should either be completed or removed.
| # Path to workspace | |
| # (Line removed) |
5ba9d6e to
e5afcb1
Compare
fixing the dynamic port for leaning mfe as for overiding the 2000 port to 2010 for all the mfe repos
e5afcb1 to
ac9f005
Compare
nsprenkle
left a comment
There was a problem hiding this comment.
NIce! I have some open questions that I think should be called out in documentation for clarity, if they aren't already. If they are, could you please point me to them?
- Where do I override this port if I want to set it to something other than
2010? - Do I have to manually run the
updatecommand to propagate all those port changes, or does this happen automatically everydev.up?
You'll also need to update the corresponding entry in the forwardPorts array and portsAttributes section to match your new port.
When the dev container is first created (postCreateCommand) However, if you change the port in an existing running container, you'll need to either: Rebuild the dev container (recommended - ensures all changes are applied) |
|
Documented As Requested |
Description
This PR adds dynamic port for LEARNING MFE in form of ENV variable
It is done in this manner so that by just changing the value here, it will update all required frontend-repositories.
And take the value dynamically.
Jira Link
[BOMS-27]
Document Link
Confluence Link