-
Notifications
You must be signed in to change notification settings - Fork 12
fix: Dynamic port for LEARNING MFE in form of ENV variable #164
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable LEARNING_MICROFRONTEND_NETLOC is used but not defined in this file. This will cause a NameError when the code runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
| 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') |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The os module is used but not imported. Add 'import os' at the top of the file to avoid a NameError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
| echo "Updating .env.development files for MFE apps..." | ||
|
|
||
| # Define shared values | ||
| PORT=${LEARNING_MICROFRONTEND_PORT:-2000} |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
| PORT=${LEARNING_MICROFRONTEND_PORT:-2000} | ||
| BASE_URL=${LEARNING_MICROFRONTEND_URL:-"http://localhost:$PORT"} | ||
|
|
||
| # Path to workspace |
Copilot
AI
Jul 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment appears incomplete and doesn't describe any actual code. It should either be completed or removed.
| # Path to workspace | |
| # (Line removed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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