-
Notifications
You must be signed in to change notification settings - Fork 107
feat: add frontend-base support #285
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| name: Update site lockfile | ||
|
|
||
| on: | ||
| schedule: | ||
| - cron: "0 2 * * *" | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| update: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| - name: Install tutor (main branch) and this plugin | ||
| run: | | ||
| pip install git+https://github.com/overhangio/tutor.git@main | ||
| pip install -e . | ||
| - name: Render tutor templates | ||
| run: | | ||
| tutor config save | ||
| - name: Regenerate @openedx/ scoped entries in site lockfile | ||
| run: > | ||
| tutor mfe update-site-lockfile | ||
| --scope @openedx/ | ||
| --output tutormfe/templates/mfe/build/mfe/site/package-lock.json | ||
| - name: Open pull request | ||
| uses: peter-evans/create-pull-request@v6 | ||
| with: | ||
| branch: auto/update-site-lockfile | ||
| title: "chore: update @openedx/ entries in site package-lock.json" | ||
| commit-message: "chore: update @openedx/ entries in site package-lock.json" | ||
| body: | | ||
| Automated update of `@openedx/`-scoped entries in the frontend-base site's `package-lock.json`, regenerated via `tutor mfe update-site-lockfile --scope @openedx/`. |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| - [Feature] Add support for [frontend-base](https://github.com/openedx/frontend-base), a unified framework that bundles frontend apps into a single shell application. Introduces a new `FRONTEND_APPS` hook and ships four core apps (`authn`, `learner-dashboard`, `instructor-dashboard`, `notifications`), with the latter two enabled by default. (by @arbrandes and @holaontiveros) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import os | ||
| import shutil | ||
| import tempfile | ||
|
|
||
| import click | ||
| from tutor import env, exceptions, fmt, images | ||
| from tutor.commands.context import Context | ||
|
|
||
|
|
||
| @click.group(name="mfe", help="Commands for the MFE plugin.") | ||
| def mfe_command() -> None: | ||
| pass | ||
|
|
||
|
|
||
| @mfe_command.command( | ||
| name="update-site-lockfile", | ||
| help=( | ||
| "Regenerate the frontend-base site's package-lock.json with the latest " | ||
| "versions allowed by the declared semver ranges. Requires `tutor config " | ||
| "save` to have been run first." | ||
| ), | ||
| ) | ||
| @click.option( | ||
| "-o", | ||
| "--output", | ||
| type=click.Path(dir_okay=False, writable=True, resolve_path=True), | ||
| default="package-lock.json", | ||
| show_default=True, | ||
| help="Path to write the refreshed lockfile to.", | ||
| ) | ||
| @click.option( | ||
| "--scope", | ||
| "scopes", | ||
| multiple=True, | ||
| help=( | ||
| "npm scope prefix to limit updates to (e.g. '@openedx/'). May be " | ||
| "passed multiple times. If neither --scope nor --package is given, " | ||
| "all dependencies are updated." | ||
| ), | ||
| ) | ||
| @click.option( | ||
| "--package", | ||
| "packages", | ||
| multiple=True, | ||
| help=( | ||
| "Exact package name to update. May be passed multiple times and " | ||
| "combined with --scope." | ||
| ), | ||
| ) | ||
| @click.pass_obj | ||
| def update_site_lockfile( | ||
| context: Context, | ||
| output: str, | ||
| scopes: tuple[str, ...], | ||
| packages: tuple[str, ...], | ||
| ) -> None: | ||
| build_path = env.pathjoin(context.root, "plugins", "mfe", "build", "mfe") | ||
| if not os.path.isdir(build_path): | ||
| raise exceptions.TutorError( | ||
| f"Rendered MFE build directory not found at {build_path}. " | ||
| "Run `tutor config save` first." | ||
| ) | ||
| selected = _resolve_packages(build_path, scopes, packages) | ||
| with tempfile.TemporaryDirectory() as tmp: | ||
| images.build( | ||
| build_path, | ||
| "tutor-mfe-lockfile:refresh", | ||
| "--target=site-lockfile", | ||
| "--no-cache-filter=site-lockfile-builder", | ||
| f"--build-arg=NPM_UPDATE_PACKAGES={' '.join(selected)}", | ||
| f"--output=type=local,dest={tmp}", | ||
| ) | ||
| src = os.path.join(tmp, "package-lock.json") | ||
| if not os.path.exists(src): | ||
| raise exceptions.TutorError( | ||
| "Docker build did not produce a package-lock.json. " | ||
| "Ensure at least one frontend app is enabled via FRONTEND_APPS." | ||
| ) | ||
| shutil.copy(src, output) | ||
| fmt.echo_info(f"Refreshed lockfile written to {output}") | ||
|
|
||
|
|
||
| def _resolve_packages( | ||
| build_path: str, scopes: tuple[str, ...], packages: tuple[str, ...] | ||
| ) -> list[str]: | ||
| if not scopes and not packages: | ||
| return [] | ||
| pkg_path = os.path.join(build_path, "site", "package.json") | ||
| try: | ||
| with open(pkg_path, encoding="utf-8") as f: | ||
| pkg = json.load(f) | ||
| except FileNotFoundError as e: | ||
| raise exceptions.TutorError( | ||
| f"Rendered site package.json not found at {pkg_path}." | ||
| ) from e | ||
| declared = { | ||
| *pkg.get("dependencies", {}).keys(), | ||
| *pkg.get("devDependencies", {}).keys(), | ||
| } | ||
| selected = { | ||
| name | ||
| for name in declared | ||
| if name in packages or any(name.startswith(s) for s in scopes) | ||
| } | ||
| if not selected: | ||
| raise exceptions.TutorError( | ||
| "No dependencies matched the given --scope / --package filters." | ||
| ) | ||
| return sorted(selected) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| {% if is_frontend_app_enabled("notifications") %} | ||
| NOTIFICATIONS_DEFAULT_FROM_EMAIL: '{{ NOTIFICATIONS_DEFAULT_FROM_EMAIL }}' | ||
| {% endif %} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| {#- TODO(legacy-mfe-removal): drop mfe_data, the mfe_data.mounted loop below, | ||
| and the mfe_data.unmounted / legacy port-mapping block further down. Only | ||
| the mfe-dev site block survives. #} | ||
| {%- set mfe_data = MFEMountData(MOUNTS) %} | ||
| {%- set site_mounts = get_site_mounts(MOUNTS) %} | ||
|
|
||
| {%- for app_name, app, mounts in mfe_data.mounted %} | ||
| {{ app_name }}: # Work on this MFE for development | ||
|
|
@@ -19,8 +23,25 @@ | |
| - "PORT={{ app['port'] }}" | ||
| {%- endfor %} | ||
|
|
||
| {%- if site_mounts %} | ||
| mfe-dev: # Work on the frontend site for development | ||
| image: "{{ MFE_DOCKER_IMAGE_DEV_PREFIX }}-mfe-dev:{{ MFE_VERSION }}" | ||
| ports: | ||
| - "{{ MFE_SITE_PORT }}:{{ MFE_SITE_PORT }}" | ||
| stdin_open: true | ||
| tty: true | ||
| volumes: | ||
| {%- for mount in site_mounts %} | ||
| - {{ mount }} | ||
| {%- endfor %} | ||
| restart: unless-stopped | ||
| depends_on: | ||
| - lms | ||
| environment: | ||
| - "PORT={{ MFE_SITE_PORT }}" | ||
| {%- endif %} | ||
|
|
||
| {% if mfe_data.unmounted|length > 0 or MFE_HOST_EXTRA_FILES %} | ||
| {% if mfe_data.unmounted|length > 0 or not site_mounts or MFE_HOST_EXTRA_FILES %} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious about this logic. I think the main thing that is making it seem confusing to me is that I see 2 possible solutions here:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not necessarily, if you take it to mean: "all of the site-related mounts that go into the On the suggestions:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. So with if is_frontend_app_enabled(app_name):
mounts.append(("mfe", f"frontend-app-{app_name}-src"))
mounts.append(("mfe-dev", f"frontend-app-{app_name}-src"))the Seems like a classic "names are hard" problem. When I first saw
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually looking into what we can do, here. Be back in a bit. :)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I don't have a better idea for the name (what I commented earlier holds), but you did help clarify the implementation: |
||
| mfe: | ||
| ports: | ||
| {%- if MFE_HOST_EXTRA_FILES %} | ||
|
|
@@ -29,4 +50,7 @@ mfe: | |
| {%- for app_name, app in mfe_data.unmounted %} | ||
| - {{ app["port"] }}:8002 # {{ app_name }} | ||
| {%- endfor %} | ||
| {%- if not site_mounts and get_frontend_apps() %} | ||
| - {{ MFE_SITE_PORT }}:8002 # site | ||
| {%- endif %} | ||
| {% endif %} | ||
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.
Overall this workflow makes sense to me, but I'm slightly concerned that it won't catch build failures with the updated lockfile.
I know in theory things should stay compatible with our pinning strategy, but I've run into issues where dependencies unintentionally release a breaking change as minor (most recently openedx/paragon#4218 / openedx/paragon#4257)
With renovate PRs this gets caught by CI, but I don't see anything in
.github/workflows/test.ymlor thetesttarget in theMakefilethat would catch things like that.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.
Agreed, but the problem is that tutor mfe has never run CI against the actual build, probably because it's too expensive. Probably good for a follow-up conversation with the other maintainers.
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.
(Or it's just that this already runs as part of the Docker build process somewhere else not visible from here - which I think is the likely answer.)