fix: render mfe-ports for unmounted mfes when these exist #241
Conversation
| {%- set _ = unmounted_mfes.append((app_name, app)) %} | ||
| {%- endif %} | ||
| {%- endfor %} | ||
| {% if unmounted_mfes|length > 0 %} |
There was a problem hiding this comment.
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.
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.
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.
Right. In which case, I suggest to expose a unmounted_mfes variable via the ENV_TEMPLATE_VARIABLES filter.
There was a problem hiding this comment.
I've tried to applied using the below code.
def get_unmounted_mfes() -> list:
unmounted_mfes = []
def append_to_unmounted_mfes(mounts: list[str], app_name:str, app: MFE_ATTRS_TYPE):
if not mounts:
unmounted_mfes.append((app_name, app))
return unmounted_mfes
return append_to_unmounted_mfes
tutor_hooks.Filters.ENV_TEMPLATE_VARIABLES.add_items(
[
...
("get_unmounted_mfes", get_unmounted_mfes()),
]
)That leads us to this template:
{%- set unmounted_mfes = [] %}
{%- for app_name, app in iter_mfes() %}
{%- set mounts = iter_mounts(MOUNTS, app_name)|list %}
{%- set unmounted_mfes = get_unmounted_mfes(mounts, app_name, app) %} ---> here I add this line
{%- if mounts %}
{{ app_name }}: # Work on this MFE for development
...
{%- endif %}
{%- endfor %}
{% if unmounted_mfes|length > 0 %}
mfe:
ports:
{%- for app_name, app in unmounted_mfes %}
- {{ app["port"] }}:8002 # {{ app_name }}
{%- endfor %}
{% endif %}
There was a problem hiding this comment.
I have re-arranged the code in the patch.
There was a problem hiding this comment.
I'm not a big fan of this pattern, as it moves too much declarative code to the Jinja templates. This is not a great practice. Instead, I suggest:
def get_unmounted_mfes(mounts):
unmounted_mfes = []
for mfe in ...:
if ...:
unmounted_mfes.append(mfe)
return unmounted_mfes
tutor_hooks.Filters.ENV_TEMPLATE_VARIABLES.add_item(
("get_unmounted_mfes", get_unmounted_mfes),
)
and then in the templates:
{%- set unmounted_mfes = get_unmounted_mfes(MOUNTS) %}
| {%- 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.
shouldn't append be enough by itself? Is it necessary to set the output to _?
There was a problem hiding this comment.
The set _ is necessary. Jinja does not support direct in-place modifications like Python does (list.append()), so you need to reassign the list with the new value.
https://stackoverflow.com/a/66019113
Description
The issue occurs when all MFEs (Micro Frontends) are mounted, and the
portssection 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 devcontext, 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 savetutor dev startOR directly check themfe-portssection intutor-env/env/dev/docker-compose.ymlWhat 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