Skip to content

[AU-2523] codespaces devstack collision port 2000 for frontend-app-learning #150

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

papphelix
Copy link
Member

Due to github codespace mapping system port 2000 & had a collision with make dev.up we had to fix the ports for frontend-app-learning

  • Fixed docker-compose.yml port mapping for frontend-app-learning to 2010
  • Updated referred files for cors or request site mapping
  • Updated documentation
  • Updated devcontainer.json for codepace forward port mapping

@papphelix papphelix changed the title Au 2523 codespaces devstack collision port 2000 [AU-2523] codespaces devstack collision port 2000 for frontend-app-learning Jun 3, 2025
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

  1. Let's ensure others are on the same page about the port change. (I'll help with that.)
  2. Then we can communicate out when we land this.
    Thank you.

@@ -31,7 +31,7 @@ Instead of a service name or list, you can also run commands like ``make dev.pro
+------------------------------------+-------------------------------------+----------------+--------------+
| `frontend-app-learner-record`_ | http://localhost:1990/ | MFE (React.js) | Default |
+------------------------------------+-------------------------------------+----------------+--------------+
| `frontend-app-learning`_ | http://localhost:2000/ | MFE (React.js) | Default |
| `frontend-app-learning`_ | http://localhost:2010/ | MFE (React.js) | Default |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want a row with "Do not use. Reserved for Codespaces.", and http://localhost:2000/, to help ensure this doesn't happen again in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a row in rst table and added explanation on the same

@papphelix papphelix requested a review from robrap June 5, 2025 13:16

Some common service combinations include:

* ``lms``: LMS, along with dependencies ``forum``, ``discovery``, ``Authn`` and some databases
* ``ecommerce``: Ecommerce, but also LMS as a dependency (for auth)
* ``cms+credentials``: Services can be combined to affect both at once
* **Reserved for Codespace**: The port `2000` is being used for github Codespace service & reserverd for Codespace use only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* **Reserved for Codespace**: The port `2000` is being used for github Codespace service & reserverd for Codespace use only.
* **Reserved for Codespace**: The port `2000` is being used for github Codespace service & reserved for Codespace use only.

@@ -1103,7 +1103,7 @@ services:
aliases:
- edx.devstack.frontend-app-learning
ports:
- "2000:2000"
- "2010:2000" # avoid Codespaces port 2000 conflict
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment and the notes in the rst doc are probably enough. I do not think we need to document this conflict for every usage of http://localhost:2010. We'll get an additional opinion before deciding either way, but it will be important to decide before making changes across repos.

@@ -18,7 +18,7 @@
'http://localhost:1991', # frontend-app-admin-portal
'http://localhost:18400', # frontend-app-publisher
'http://localhost:18450', # frontend-app-support-tools
'http://localhost:2000', # frontend-app-learning
'http://localhost:2010', # frontend-app-learning (port changed from 2000 to 2010, due to conflict in codepsace)
Copy link
Contributor

Choose a reason for hiding this comment

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

See earlier comment about whether or not we might drop all these redundant comment changes.

Copy link
Contributor

@robrap robrap Jun 5, 2025

Choose a reason for hiding this comment

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

  1. I got confirmation in Slack that we can remove these additional comments.
  2. Before this merges, we'll need to prep PRs for the following: https://github.com/search?q=org%3Aedx+localhost%3A2000++NOT+is%3Aarchived&type=code
  3. We'll also need a communication plan before we merge as well.

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants