-
Notifications
You must be signed in to change notification settings - Fork 96
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
fix: render mfe-ports for unmounted mfes when these exist #241
base: release
Are you sure you want to change the base?
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.
This solution is way better than the one I initially suggested. Please just add a changelog entry and add "close #242" to your commit message.
{%- set _ = unmounted_mfes.append((app_name, app)) %} | ||
{%- endif %} | ||
{%- endfor %} | ||
{% if unmounted_mfes|length > 0 %} |
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.
nit: {% if unmounted_mfes|length > 0 %}
@@ -1,10 +1,17 @@ | |||
{%- set unmounted_mfes = [] %} | |||
{%- for app_name, app in iter_mfes() %} |
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.
Could we simplify this declaration with the following?
{%- set unmounted_mfes = [(app_name, app) for app_name, app in iter_mfes() if not list(iter_mounts(MOUNTS, app_name))] %}
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.
It's giving error: Error: Template syntax error: expected token ',', got 'for'
. Because of ,
in app_name, app
. I've also tried with iter_mfes().items()
. Got same error.
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.
Right. In which case, I suggest to expose a unmounted_mfes
variable via the ENV_TEMPLATE_VARIABLES filter.
{%- set unmounted_mfes = [] %} | ||
{%- for app_name, app in iter_mfes() %} | ||
{%- if not iter_mounts(MOUNTS, app_name)|list %} | ||
{%- set _ = unmounted_mfes.append((app_name, app)) %} |
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.
shouldn't append be enough by itself? Is it necessary to set the output to _?
Description
The issue occurs when all MFEs (Micro Frontends) are mounted, and the
ports
section in the Docker configuration is left empty, leading to a syntax error in the generated Dockerfile. Specifically, theports:
line becomes invalid as another key follows it (auth:
) without any content. In thetutor dev
context, the issue occurs because developers mount MFEs for testing purposes. It won't reproduce fortutor local
.How to Reproduce
authn, account, communications, course-authoring, discussions, gradebook, learner-dashboard, learning, ora-grading, profile
)tutor mounts add $(pwd)/frontend-app-*
where*
represents all MFEs name one by onetutor config save
tutor dev start
OR directly check themfe-ports
section intutor-env/env/dev/docker-compose.yml
What this PR does
This PR resolves the issue by implementing logic to first check if any unmounted MFEs exist. If unmounted MFEs are found, their corresponding ports are rendered in the ports: section. This ensures that the Docker configuration is generated correctly and avoids errors caused by leaving the
ports:
section empty.TestCases:
mfe: ports:
section.mfe: ports:
section should be excluded entirely from the generated output.Closes #242