Skip to content
11 changes: 10 additions & 1 deletion tutormfe/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,17 @@

from tutor.core.hooks import Filter

MFE_ATTRS_TYPE = t.Dict[t.Literal["repository", "port", "version"], t.Union["str", int]]
MFE_ATTRS_TYPE = t.Dict[
t.Literal["repository", "port", "version"], t.Union["str", int]
]
FRONTEND_TEMPLATE_SITE_ATTRS_TYPE = t.Dict[

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unless I misunderstood the purpose, this is about individual apps, not the whole site, right?

Suggested change
FRONTEND_TEMPLATE_SITE_ATTRS_TYPE = t.Dict[
FRONTEND_APP_ATTRS_TYPE = t.Dict[

t.Literal["repository", "version"], t.Union["str", int]
]

MFE_APPS: Filter[dict[str, MFE_ATTRS_TYPE], []] = Filter()

# TODO: This will hold the list of which apps are "enabled" so we can switch between mfe
# and frontend-base ones
FRONTEND_APPS: Filter[dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE], []] = Filter()

PLUGIN_SLOTS: Filter[list[tuple[str, str, str]], []] = Filter()
116 changes: 110 additions & 6 deletions tutormfe/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
from glob import glob

import importlib_resources
from tutor import fmt
from tutor import fmt, config as tutor_config
from tutor import hooks as tutor_hooks
from tutor.__about__ import __version_suffix__
from tutor.bindmount import iter_mounts
from tutor.hooks import priorities
from tutor.types import Config, get_typed

from .__about__ import __version__
from .hooks import MFE_APPS, MFE_ATTRS_TYPE, PLUGIN_SLOTS
from .hooks import MFE_APPS, MFE_ATTRS_TYPE, FRONTEND_APPS, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE, PLUGIN_SLOTS

# Handle version suffix in main mode, just like tutor core
if __version_suffix__:
Expand Down Expand Up @@ -76,6 +76,11 @@
"repository": "https://github.com/openedx/frontend-app-profile.git",
"port": 1995,
},
# "template-site": {
# "repository": "https://github.com/WGU-Open-edX/frontend-template-site.git",
# "version": "initial",
# "port": 8080,
# }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we remove this commented out bit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had it to remember to add the template site as a default app, BUT if we are going with base files to be patched then yeah I'll just kill this part

}


Expand All @@ -95,6 +100,49 @@ def get_mfes() -> dict[str, MFE_ATTRS_TYPE]:
return MFE_APPS.apply({})


# List will need
## Apps that are only frontend-apps
## Apps that are only MFEs
## Apps with unique ones (all old mfes + instruct)
## 1 and 2 with 1 having something like a different identifier
Comment on lines +103 to +107

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Without reading the code, this comment is hard to understand. What are "apps with unique ones"? What are 1 and 2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, It was for my own read when I was iteraring through different approachces, I'll clean it as soon as we get to the final draft



@tutor_hooks.lru_cache
def get_frontend_apps(apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

apps_to_build sounds like a list, but apparently it's a boolean. Let's call it something like only_apps_to_build, then?

Suggested change
def get_frontend_apps(apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]:
def get_frontend_apps(only_apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]:

"""
This function is cached for performance.
"""
all_frontend_apps = FRONTEND_APPS.apply({})

if not apps_to_build:
return all_frontend_apps

# When returning apps to build we only return the ones that have a repository defined
# (those are the ones to be built) and we prefix the name
# with "frontend-app-" to avoid conflicts with MFE names
return {
f"frontend-app-{name}": attrs
for name, attrs in all_frontend_apps.items()
if "repository" in attrs
}


@tutor_hooks.lru_cache
def get_all_apps() -> dict[str, t.Union[MFE_ATTRS_TYPE, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not literally getting all apps. Just the ones that need to be build locally:

Suggested change
def get_all_apps() -> dict[str, t.Union[MFE_ATTRS_TYPE, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:
def get_mfes_and_apps_to_build() -> dict[str, t.Union[MFE_ATTRS_TYPE, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:

"""
This function is cached for performance.
"""
# IMPORTANT: Make a copy to avoid mutating the cached result from get_frontend_apps()
all_apps = get_frontend_apps(apps_to_build=True).copy()
mfes = get_mfes()
all_apps.update(mfes)
Comment on lines +136 to +138

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: this makes it feel like the frontend apps are "more default", when it's really the other way around. For now, I think we should populate the list with the regular MFEs first. Also, I feel like we should distinguish more clearly between MFEs and apps.

Suggested change
all_apps = get_frontend_apps(apps_to_build=True).copy()
mfes = get_mfes()
all_apps.update(mfes)
mfes_and_apps = get_mfes()
apps_to_build = get_frontend_apps(only_apps_to_build=True).copy()
mfes_and_apps.update(apps_to_build)


# ensure frontend-template-site is the last one on the all_apps dict
if "template-site" in all_apps:
all_apps["template-site"] = all_apps.pop("template-site")
Comment on lines +141 to +142

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if "template-site" in all_apps:
all_apps["template-site"] = all_apps.pop("template-site")
if "frontend-site" in mfes_and_apps:
mfes_and_apps["frontend-site"] = mfes_and_apps.pop("frontend-site")


return all_apps

class MFEMountData:
"""Stores categorized mounted and unmounted MFEs."""

Expand Down Expand Up @@ -129,6 +177,50 @@ def iter_mfes() -> t.Iterable[tuple[str, MFE_ATTRS_TYPE]]:
"""
yield from get_mfes().items()

# Iter throgh all mfes and adds the unique frontend apps,
# so we can have a list of all the things that are unique that needs
# to be added to Caddyfile, for example instructor dashboard that was
# created as frontend-base app but didn't exist as a MFE before
# so it returns the whole mfes list plus the unique frontend apps that are not in the mfe list
Comment on lines +180 to +184

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I had to read the code to understand why the choice of "unique", here. I think it would be a good idea to explicitly describe the situation where there might be duplication, including an example.

Also, since is just for the Caddyfile, let's call the function what it is: iter_paths.

def iter_unique_apps() -> t.Iterable[tuple[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def iter_unique_apps() -> t.Iterable[tuple[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:
def iter_paths() -> t.Iterable[tuple[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:

"""
Yield:

(name, dict)
"""
mfes = get_mfes()
frontend_apps = get_frontend_apps()

# First yield all MFEs
for name, attrs in mfes.items():
yield (name, attrs)

# Then yield frontend apps that are not already MFEs
for name, attrs in frontend_apps.items():
if name not in mfes:
yield (name, attrs)

# Iters through all apps that will be built
def iter_all_apps() -> t.Iterable[tuple[str, t.Union[MFE_ATTRS_TYPE, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]]:
"""
Yield:

(name, dict)
"""
all_apps = get_all_apps()
for name, attrs in all_apps.items():
yield (name, attrs)

def iter_frontend_apps_to_build() -> t.Iterable[tuple[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]]:
"""
Yield:

(name, dict)
"""
frontend_apps = get_frontend_apps(apps_to_build=True)
for name, attrs in frontend_apps.items():
yield (name, attrs)


def iter_plugin_slots(mfe_name: str) -> t.Iterable[tuple[str, str]]:
"""
Expand All @@ -142,6 +234,11 @@ def iter_plugin_slots(mfe_name: str) -> t.Iterable[tuple[str, str]]:
def is_mfe_enabled(mfe_name: str) -> bool:
return mfe_name in get_mfes()

def is_frontend_app_enabled(app_name: str) -> bool:
return app_name in get_frontend_apps()

def is_frontend_app_to_build(app_name: str) -> bool:
return app_name in get_frontend_apps(apps_to_build=True)

def get_mfe(mfe_name: str) -> t.Union[MFE_ATTRS_TYPE, t.Any]:
return get_mfes().get(mfe_name, {})
Expand All @@ -152,8 +249,13 @@ def get_mfe(mfe_name: str) -> t.Union[MFE_ATTRS_TYPE, t.Any]:
[
("get_mfe", get_mfe),
("iter_mfes", iter_mfes),
("iter_unique_apps", iter_unique_apps),
("iter_all_apps", iter_all_apps),
("iter_frontend_apps_to_build", iter_frontend_apps_to_build),
("iter_plugin_slots", iter_plugin_slots),
("is_mfe_enabled", is_mfe_enabled),
("is_frontend_app_enabled", is_frontend_app_enabled),
("is_frontend_app_to_build", is_frontend_app_to_build),
("MFEMountData", MFEMountData),
]
)
Expand Down Expand Up @@ -217,6 +319,8 @@ def _mounted_mfe_image_management() -> None:
tutor_hooks.Filters.CLI_DO_INIT_TASKS.add_item(("lms", task_file.read()))

REPO_PREFIX = "frontend-app-"
# TODO: for now leave this and then find better semantic namings
FRONTEND_TEMPLATE_SITE_PREFIX = "frontend-"


@tutor_hooks.Filters.COMPOSE_MOUNTS.add()
Expand All @@ -229,12 +333,12 @@ def _mount_frontend_apps(
in dev mode, because in production, all MFEs are built and hosted on the
singular 'mfe' service container.
"""
if path_basename.startswith(REPO_PREFIX):
if path_basename.startswith(REPO_PREFIX) or path_basename.startswith(FRONTEND_TEMPLATE_SITE_PREFIX):
# Assumption:
# For each repo named frontend-app-APPNAME, there is an associated
# docker-compose service named APPNAME. If this assumption is broken,
# then Tutor will try to mount the repo in a service that doesn't exist.
app_name = path_basename[len(REPO_PREFIX) :]
app_name = path_basename[len(REPO_PREFIX) :] if path_basename.startswith(REPO_PREFIX) else path_basename[len(FRONTEND_TEMPLATE_SITE_PREFIX) :]
volumes += [(app_name, "/openedx/app")]
return volumes

Expand All @@ -244,9 +348,9 @@ def _mount_frontend_apps_on_build(
mounts: list[tuple[str, str]], host_path: str
) -> list[tuple[str, str]]:
path_basename = os.path.basename(host_path)
if path_basename.startswith(REPO_PREFIX):
if path_basename.startswith(REPO_PREFIX) or path_basename.startswith(FRONTEND_TEMPLATE_SITE_PREFIX):
# Bind-mount repo at build-time, both for prod and dev images
app_name = path_basename[len(REPO_PREFIX) :]
app_name = path_basename[len(REPO_PREFIX) :] if path_basename.startswith(REPO_PREFIX) else path_basename[len(FRONTEND_TEMPLATE_SITE_PREFIX) :]
mounts.append(("mfe", f"{app_name}-src"))
mounts.append((f"{app_name}-dev", f"{app_name}-src"))
return mounts
Expand Down
9 changes: 8 additions & 1 deletion tutormfe/templates/mfe/apps/mfe/Caddyfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,22 @@
redir @authoring /authoring/{re.authoring.1} permanent
{% endif %}

{% for app_name, app in iter_mfes() %}
{% for app_name, app in iter_unique_apps() %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(See the other comment about the name of this function)

Suggested change
{% for app_name, app in iter_unique_apps() %}
{% for path, app in iter_paths() %}

@mfe_{{ app_name }} {
path /{{ app_name }} /{{ app_name }}/*
}
handle @mfe_{{ app_name }} {
uri strip_prefix /{{ app_name }}
{%- if is_frontend_app_enabled(app_name) %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shorter and clearer, I think:

Suggested change
{%- if is_frontend_app_enabled(app_name) %}
{%- if is_frontend_app(path) %}

# {{ app_name }} - using frontend-apps approach
root * /openedx/dist/template-site

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How about calling it frontend-site?

Suggested change
root * /openedx/dist/template-site
root * /openedx/dist/frontend-site

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, this is currently based on the naming for the especific repo, so given that it's called "template-site" it ends up with that name, so it's a matter of changing the config

{%- else %}
# {{ app_name }} - using traditional MFE approach
root * /openedx/dist/{{ app_name }}
{%- endif %}
try_files /{path} /index.html
file_server
}

{% endfor %}
}
24 changes: 21 additions & 3 deletions tutormfe/templates/mfe/build/mfe/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ENV PATH=/openedx/app/node_modules/.bin:${PATH}

{{ patch("mfe-dockerfile-base") }}

{% for app_name, app in iter_mfes() %}
{% for app_name, app in iter_all_apps() %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For clarity, as suggested elsewhere:

Suggested change
{% for app_name, app in iter_all_apps() %}
{% for app_name, app in iter_mfes_and_apps_to_build() %}

####################### {{ app_name }} MFE
######## {{ app_name }} (git)
FROM base AS {{ app_name }}-git
Expand Down Expand Up @@ -50,9 +50,11 @@ RUN --mount=type=cache,target=/root/.npm,sharing=shared npm clean-install --no-a
{{ patch("mfe-dockerfile-post-npm-install-{}".format(app_name)) }}
COPY --from={{ app_name }}-src / /openedx/app

# While we figure out how translations will be managed in template site
# if it's either template-site or starts with frontend-app, we skip pulling translations
{% if app_name != "template-site" and not app_name.startswith("frontend-app") %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of distinguishing between MFE and frontend app just based on a string prefix. I think we should own up to the difference and have a separate loop just for frontend apps. For example, I suspect apps don't need the pre- and post-npm-install patches.

Also, I don't think frontend apps will be subject to the same MFE_COMMON_VERSION (we're not tagging frontend-base apps with release/verawood.1). This is another reason to have a separate list just for them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can definitely separate them, I just didn't want to start with something separated without having everything rolling and being able to know how we can "optimize it"

I mean so the new separate loop does only the things we know we want it to do

RUN make OPENEDX_ATLAS_PULL=true ATLAS_OPTIONS="--repository={{ ATLAS_REPOSITORY }} --revision={{ ATLAS_REVISION }} {{ ATLAS_OPTIONS }}" pull_translations

EXPOSE {{ app['port'] }}
{% endif %}

# Configuration needed at build time
ENV APP_ID={{ app_name }}
Expand All @@ -66,6 +68,12 @@ COPY env.config.jsx /openedx/app
{{ patch("mfe-dockerfile-pre-npm-build") }}
{{ patch("mfe-dockerfile-pre-npm-build-{}".format(app_name)) }}

{% if is_frontend_app_to_build(app_name) %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is_frontend_app() should be sufficient because this list only contains things to build already, but as noted in the above comment, I actually think we should have a separate loop just for frontend apps and avoid the check altogether.

RUN npm pack && mv *.tgz {{app_name}}.tgz
{% else %}
EXPOSE {{ app['port'] }}
{% endif %}

######## {{ app_name }} (dev)
FROM {{ app_name }}-common AS {{ app_name }}-dev
ENV NODE_ENV=development
Expand All @@ -76,6 +84,16 @@ CMD ["/bin/bash", "-c", "npm run start --- --config ./webpack.dev-tutor.config.j
{%- for app_name, app in iter_mfes() %}
######## {{ app_name }} (production)
FROM {{ app_name }}-common AS {{ app_name }}-prod

{% if app_name == "template-site" %}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The frontend site should not be an item on a configurable list. It should be a hard-coded part of the Dockerfile.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we'll have package.json, and a site.config.build.tsx tempaltes that we'll use as a base always and patch on top of it?

RUN mkdir -p /openedx/app/pack
{%- for app_name, app in iter_frontend_apps_to_build() %}
COPY --from={{ app_name }}-common /openedx/app/{{app_name}}.tgz /openedx/app/pack/
RUN npm install /openedx/app/pack/{{app_name}}.tgz --no-audit --no-fund
{% endfor %}
RUN npm ci
{% endif %}

ENV NODE_ENV=production
RUN npm run build
{{ patch("mfe-dockerfile-post-npm-build") }}
Expand Down
Loading