-
Notifications
You must be signed in to change notification settings - Fork 101
[WIP] fix: add script to install node_modules in mounted mfes #232
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
base: release
Are you sure you want to change the base?
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,11 @@ | ||
{%- for app_name, app in iter_mfes() %} | ||
{%- set mounts = iter_mounts(MOUNTS, app_name)|list %} | ||
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. We moved from iter_mounts to a cleaner MFEMountData object in #241. We should use the similar format here for consistency. |
||
{%- if mounts %} | ||
{{ app_name }}-job: | ||
image: "{{ MFE_DOCKER_IMAGE_DEV_PREFIX }}-{{ app_name }}-dev:{{ MFE_VERSION }}" | ||
volumes: | ||
{%- for mount in mounts %} | ||
- {{ mount }} | ||
{%- endfor %} | ||
{%- endif %} | ||
{%- endfor %} | ||
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. nit: add extra line at end of file |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{%- for app_name, app in iter_mfes() %} | ||
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. Why is this patch necessary? Don't we just need to run the init job in dev? |
||
{%- set mounts = iter_mounts(MOUNTS, app_name)|list %} | ||
{%- if mounts %} | ||
{{ app_name }}-job: | ||
image: "{{ MFE_DOCKER_IMAGE }}" | ||
{%- endif %} | ||
{%- endfor %} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import functools | ||
import os | ||
import re | ||
import typing as t | ||
from glob import glob | ||
|
||
|
@@ -304,3 +305,30 @@ def _check_mfe_host(config: Config) -> None: | |
f'Warning: MFE_HOST="{mfe_host}" is not a subdomain of LMS_HOST="{lms_host}". ' | ||
"This configuration is not typically recommended and may lead to unexpected behavior." | ||
) | ||
|
||
|
||
@tutor_hooks.Actions.CONFIG_LOADED.add() | ||
def _run_jobs_in_mounted_mfes(config: Config) -> None: | ||
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 not a big fan of adding callbacks to the CONFIG_LOADED action. It means we have to manually parse Instead, I suggest the following:
What do you think? |
||
mounts = get_typed(config, "MOUNTS", list, []) | ||
|
||
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. We can return here if no mounts exist in the config. 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. +1 |
||
if not mounts: | ||
return None | ||
|
||
pattern = re.compile(rf'{re.escape(REPO_PREFIX)}(\w+)') | ||
mounted_mfes = [match.group(1) for mount in mounts if (match := pattern.search(mount))] | ||
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. Looking back, we should be able to use MFEMountData here as well instead of using a regex. What do you think? |
||
|
||
mfe_npm_install_file = os.path.join( | ||
str( | ||
importlib_resources.files("tutormfe") | ||
/ "templates" | ||
/ "mfe" | ||
/ "tasks" | ||
/ "mfe" | ||
/ "npm-install.sh" | ||
) | ||
) | ||
|
||
for mounted_mfe in mounted_mfes: | ||
with tutor_hooks.Contexts.app("mfe").enter(): | ||
with open(mfe_npm_install_file, encoding='utf-8') as fi: | ||
tutor_hooks.Filters.CLI_DO_INIT_TASKS.add_item((mounted_mfe , fi.read())) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
#!/bin/bash | ||
|
||
if [ "$NODE_ENV" != "development" ]; then | ||
echo "This script is intended to run only in the development environment." | ||
exit | ||
fi | ||
|
||
# Paths for the fingerprint and package-lock.json | ||
FINGERPRINT_FILE="node_modules/.fingerprint" | ||
PACKAGE_LOCK="package-lock.json" | ||
|
||
# Function to generate the SHA1 hash of the package-lock.json file | ||
generate_fingerprint() { | ||
sha1sum "$PACKAGE_LOCK" | awk '{print $1}' | ||
} | ||
|
||
# If node_modules directory is missing, install dependencies | ||
if [ ! -d "node_modules" ]; then | ||
echo "⚠️ node_modules not found! Installing dependencies..." | ||
npm clean-install | ||
|
||
# If fingerprint file is missing, assume packages may be out of sync and reinstall | ||
elif [ ! -f "$FINGERPRINT_FILE" ]; then | ||
echo "⚠️ Fingerprint file not found! Re-installing dependencies..." | ||
npm clean-install | ||
|
||
# If both node_modules and fingerprint file exist, compare fingerprints | ||
else | ||
CURRENT_FINGERPRINT=$(generate_fingerprint) | ||
STORED_FINGERPRINT=$(cat "$FINGERPRINT_FILE") | ||
|
||
# Reinstall if package-lock.json has changed | ||
if [ "$CURRENT_FINGERPRINT" != "$STORED_FINGERPRINT" ]; then | ||
echo "⚠️ package-lock.json changed! Re-installing dependencies..." | ||
npm clean-install | ||
else | ||
# Everything is up to date | ||
echo "✅ Dependencies are up to date." | ||
exit 0 | ||
fi | ||
fi | ||
|
||
|
||
# Store the new fingerprint after installation | ||
generate_fingerprint > "$FINGERPRINT_FILE" | ||
echo "✅ Dependencies installed and fingerprint updated." | ||
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. nit: add extra line at end of file |
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.
why is this patch needed?