feat: add paragon hosting configuration for dev and local environments (FC-87) #38
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @Ang-m4! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Hi @Alec4r! |
|
This PR cleanly introduces a well-scoped, dedicated Paragon static file server that enables serving compiled, minified themes through a robust and secure NGINX setup. The implementation follows Tutor plugin best practices with clear separation of concerns for dev and production environments, dynamic configuration of theme URLs, and efficient theme compilation using PostCSS. The reverse proxy configuration is precise and minimizes exposure, ensuring only the necessary assets are served. Overall, the solution is modular, maintainable, and aligns well with the platform’s architecture. |
@Ang-m4 could you elaborate on this? I agree that trying to serve the files through the existing LMS service doesn't make sense, but I'd think Caddy would be more than capable of serving the files. I also think that using Caddy would make it easier to integrate this plugin into the |
|
Hi @brian-smith-tcril , thanks for the question! Let me elaborate. The primary issue with using Caddy is that the service is only available in the Additionally, we would face challenges attaching the volume containing the CSS files to the Caddy service, as there is no straightforward Tutor patch template for that purpose. |
I could be mistaken but that sounds incorrect to me. My understanding is that when someone runs |
|
@brian-smith-tcril , I apologize for the delayed response. To clarify my initial comment, I was referring to the general Caddy service that is only available in the local environment. As you rightly pointed out, there is a Caddy-based service in the MFE plugin that serves in both dev and local environments and is explicitly configured for MFE assets. I think the approach to add a new, dedicated service is correct. We implemented an NGINX service as it's lightweight and familiar, but if you prefer Caddy for consistency with the MFE plugin, I am happy to adjust the base image. Please let me know what you think. |
No worries at all!
Thanks for that clarification! Sounds like we're on the same page about that now!
I 100% agree. Trying to hook into the Caddy service in the MFE plugin would get messy fast.
My concern is definitely coming from a "for consistency" perspective. My thought was that using Caddy in both this plugin and the It is also possible that using Caddy vs NGINX wouldn't actually change the amount of effort required to integrate the functionality from this plugin into the I'd like to loop in @arbrandes here - any thoughts on having this plugin host the generated css via NGINX vs Caddy? |
|
Hi @brian-smith-tcril, I've already tested this against the latest version of Tutor, and everything is working as expected. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall this looks great! I left a couple comments with questions but nothing major. Thank you so much for putting this together!
plugins/tutor-contrib-paragon/tutorparagon/patches/openedx-lms-development-settings
Outdated
Show resolved
Hide resolved
| build_css_bundle() { | ||
| # This function builds a CSS bundle using PostCSS. | ||
| # It takes the path to the index.css file as an argument. | ||
| # Usage: build_css_bundle <index_css_file> | ||
|
|
||
| local index_css_file="$1" | ||
|
|
||
| local bundle_directory=$(dirname "$index_css_file") | ||
| local bundle_name=$(basename "$bundle_directory") | ||
| local minified_output_file="$bundle_directory/${bundle_name}.min.css" | ||
|
|
||
| if npx postcss "$index_css_file" \ | ||
| --use postcss-import \ | ||
| --use postcss-custom-media \ | ||
| --use postcss-combine-duplicated-selectors \ | ||
| --use postcss-minify \ | ||
| --no-map \ | ||
| --output "$minified_output_file"; then | ||
|
|
||
| echo "Successfully created bundle: $bundle_name" | ||
| return 0 | ||
| else | ||
| echo "Failed to build CSS bundle: $bundle_name" >&2 | ||
| return 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
I'm not opposed to having this live here, but if we think postcss bundling paragon generated css will be a common thing then maybe it makes sense to build that functionality into the Paragon CLI. @PKulkoRaccoonGang thoughts?
- add an upper limit version for tutor dependency - create a shared template for the mfe config on both dev and local envs - update the commonly used port for the hosting service to 12400
|
@Ang-m4, @brian-smith-tcril, I'd like to revisit the idea of re-using the tutor-mfe Caddy container for this. It has a Caddyfile hook that could be used. What it doesn't have is a hook for the docker-compose service so as to add another volume, but I don't see why anybody would object to adding one here: If you submit a PR there, pointing back here as an explanation for the reason, I'd be glad to review it! |
|
@arbrandes that is great news! It sounds like it won't be as messy as I thought it might be! |
|
@arbrandes, @brian-smith-tcril, i think that it might be a great idea, but we should consider that we might need to make some more interesting changes regarding the k8s implementation. For the basic My main consideration is how we'll handle the Kubernetes env. In our current approach, we use a Also, just a quick heads-up: this change will naturally add to the scope of Milestone 4, as we'll need to document this new integration properly. It's not a blocker at all, just something to keep in mind for our timeline. |
|
My general thought is that hosting stylesheets that MFEs load at runtime falls well within the scope of the I'm fine moving forward with either approach. If the first version of this plugin handles the stylesheet hosting independently and doesn't integrate with the As for the k8s stuff I'll need to defer to @arbrandes - thoughts? |
|
@brian-smith-tcril If I remember correctly, we discussed this in the past and agreed to keep it separate from tutor-mfe to avoid delays or potential controversy. I'm okay with using Caddy, I believe it's the best approach. However, I'm not fully on board with adding any part of the tutor-paragon login logic directly into the tutor-mfe plugin. That would introduce a direct dependency from tutor-paragon to tutor-mfe, which I don't think is necessary. In the end, we'd essentially be doing the same thing: adding a reverse proxy with Caddy for local and dev environments. And for Kubernetes, we’d still need to patch this file in tutor-mfe: @Ang-m4 could you try this approach? Maybe spend up to 2 hours on it and let me know if it definitely works or if it needs further adjustments. |
Yes, but there are different levels of separation. There also wasn't as much engagement on the forum post as I would have liked to see.
This PR already includes logic that modifies the MFE config. I know that isn't a hard dependency on the The main benefit of the standalone approach in my opinion is that it allows the functionality to be built out before the conversations about what aspects of the functionality fit into the I'd still like to defer to @arbrandes, because it sounds like adding the ability to host extra css within the |
|
After taking a closer look at the implementation of the I would appreciate it if you or @arbrandes could take a look at the proposed changes, especially those for the Let me know what you think! CC. @Alec4r |
sarina
changed the title
|
Now that overhangio/tutor-mfe#264 has been merged, what additional work does this PR require? Is this the last piece of work for FC-87? |
|
Hi @sarina, @brian-smith-tcril, thanks for following up! Now that overhangio/tutor-mfe#264 has been merged, I updated this PR to add the The remaining piece for closing Milestone 3 is the Kubernetes environment integration, which I’m currently working on and will deliver shortly. Regarding Milestone 4, we have this current PR in progress: #39 |
|
Hi @sarina, @brian-smith-tcril Just a quick note: I opened a new PR overhangio/tutor-mfe#267. While working on the Kubernetes implementation, I realized that an additional patch was missing in order for the This doesn’t affect the current PR, but it is necessary for the further Kubernetes implementation. |
|
Edit: my mistake, I missed the When trying to test this locally I ran into an issue with a docker pull. Any idea why that might be happening? |
|
I'm having trouble verifying this locally when following the instructions in https://github.com/eduNEXT/openedx-tutor-plugins/blob/afg/styles-hosting-setup/plugins/tutor-contrib-paragon/README.rst I have been able to:
I haven't been able to:
I have been unsure of:
Hopefully some answers to these questions will help improve the documentation so future users of this plugin can have a smooth experience! |
|
Thanks for the detailed feedback, @brian-smith-tcril for example: Regarding when to execute the commands, I’d say it’s optional unless you want to overwrite some variables (e.g. I’ll also add this clarification as part of Milestone 4, so we make sure the documentation is clearer for future users. |
|
I was able to figure a bit more out! Things that are definitely working:
Things I still haven't seen:
After looking into the browser inspector I think I realize why this wasn't working. I noticed that the styles on the MFE didn't mention CSS variables at all. I then realized that this makes sense because in order to have overhangio/tutor-mfe@b4a783a in my local checkout of I'll do a little more testing today but I expect it to all go smoothly now that I know what to look for! Thanks for being patient with me as I figured out my local dev environment issues! |
Progress!I was able to get theme styles to load properly by:
When running 
My original thought was that this may be an upstream issue with Paragon itself and not this plugin, but I encountered a similar issue with 2U's I would expect a theme without I then decided to test out building 2U's Full outputI pulled the I then encountered a similar styling issue to the one I encountered when using the minimal theme without 
I decided to try to put the styles from the I'm not sure how much (if any) of the issues I encountered are actual problems with the plugin, and I definitely don't want to block this PR on things that are not the fault of this plugin. I do, however, think that the 2 theme examples I tried are ones where having a "happy path" documented for this plugin would be great. That doesn't need to happen before this PR can land, but it'll be good to note any findings related to getting those working with this plugin during testing. To merge this PR, I'd like to see a theme that includes
|
|
@Ang-m4 friendly ping! Just wondering if you saw my previous comment. |
|
For additional context: in prior testing I was able to get the 2U import json
from tutor import hooks
paragon_theme_urls = {
"core": {
"urls": {
"brandOverride": "https://cdn.jsdelivr.net/npm/@edx/[email protected]/dist/core.min.css"
},
},
"defaults": {
"light": "light",
},
"variants": {
"light": {
"urls": {
"default": "https://cdn.jsdelivr.net/npm/@openedx/paragon@$paragonVersion/dist/light.min.css",
"brandOverride": "https://cdn.jsdelivr.net/npm/@edx/[email protected]/dist/light.min.css"
}
}
}
}
fstring = f"""
MFE_CONFIG["PARAGON_THEME_URLS"] = {json.dumps(paragon_theme_urls)}
"""
hooks.Filters.ENV_PATCHES.add_item(
(
"mfe-lms-common-settings",
fstring
)
) |
|
Hi @brian-smith-tcril , I was able to replicate the broken views you mentioned. After checking the The issue was that I tested this using the Steps I used:
Let me know if you spot any other details or concerns, and thanks a lot for taking the time to test everything so thoroughly. 
|
|
That is great news @Ang-m4! It sounds like the one last missing puzzle piece here is that Using bundled Paragon has a few drawbacks:
This means that not using bundled Paragon should be the default. This gets a little more complex when considering the While it isn't something we hope for, it is definitely possible that MFEs in a release will have slightly different versions of Paragon bundled. Using the I do think that using |
|
@brian-smith-tcril, Got it, thanks for clarifying. I’ll update so each MFE loads the Paragon version it ships with. Quick question, I’m not that familiar with jsDelivr: can we be certain that all Paragon versions will be available there? When you say “we need to know what version of Paragon each MFE is using so we can host them all”, do you mean hosted in jsDelivr? |
Yes. If it's on
That is not what I mean, and that's what makes this tricky. Thinking this throughOne of the goals of this plugin is to allow site operators to host themes without relying on an external CDN. If we want this plugin to also support the goal of not needing to load the same version of Paragon base styles multiple times because Paragon is bundled in multiple MFEs, then we need to host those Paragon base styles here too. So the tricky part is knowing what versions we need to host. I don't know a great way to do this off the top of my head, so any suggestions are more than welcome, but I can also do a bit of digging into If we can't figure out what versions we need beforehand, I can think of a few ways to handle this:
One thing that's nice about the Of the 3 non-crossed-out options, I think 2 matches the goal the most, but would also take the most effort to implement. I also have some of the same concerns about it as I do with option 1. If we are only working with options 1 and 4, I have a hard time picking a default. Site operators that are comfortable loading styles from An ideal worldAn ideal scenario for me would be to have A path forward (aka. tl;dr - let's do this)Realistically, however, I think our best path forward is to default to option 4 (Just don't set default. Let MFEs use their bundled base styles). This ensures nobody unexpectedly hits a CDN. It will be less performant for end users than using Documentation we can provide:
I know this was a long one, thanks for being patient with me as I thought this through! |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I'm approving this as it has worked in my testing and currently implements what I described in the "tl;dr - let's do this" section of my previous comment.
I have created a follow-up issue for adding documentation for site operators that want to use non-bundled base Paragon styles #44
github-project-automation
bot
moved this from Ready for Review
to Done
in Contributions
…s (FC-87) (openedx#38) * feat: add bundle minification build step for hosting * feat: add paragon static server and caddy configuration * feat: add configuration for Paragon theme URLs in dev and local * feat: add conditional logic for serving compiled themes * feat: update static server from nginx to caddy for serving compiled themes * fix: update tutor dependency to remove upper version limit * feat: update Paragon theme configuration and static server settings - add an upper limit version for tutor dependency - create a shared template for the mfe config on both dev and local envs - update the commonly used port for the hosting service to 12400 * feat: add tutor-mfe service integration * fix: integration test dependencies * feat: add integration tests for hosted files * fix(test): update LMS_HOST variable for hosting tests * fix: update MFE_CONFIG to use 'brandOverride' for theme URLs feat: add paragon static server and caddy configuration feat: add conditional logic for serving compiled themes feat: update static server from nginx to caddy for serving compiled themes feat: update Paragon theme configuration and static server settings - add an upper limit version for tutor dependency - create a shared template for the mfe config on both dev and local envs - update the commonly used port for the hosting service to 12400 feat: add tutor-mfe service integration
7 participants
Description
This pull request introduces a Paragon static file server for the Tutor platform, enabling the serving of compiled themes. Key changes include adding a reverse proxy configuration for static files, enhancing theme compilation with PostCSS, and updating the plugin's settings to support the new server.
Key Changes
Theme compilation minimization 92ed5c9
Added PostCSS to the
package.jsondependencies to generate a minified bundle of styles, facilitating the hosting of a single file in the current themes folder. The entrypoint script was also updated to run the task based on the generated files.Add new Nginx service for hosting the files f8ae77a
Added a new service to expose the minified files via NGINX by mounting the
COMPILED_THEMESvolume. This enables using the styles in both the dev and local environments. In production, the service remains internal to the Docker Compose network and isn’t exposed externally.We’ve decided to proceed with a dedicated NGINX service due to the complexity of serving files through the existing LMS or Caddy services. It’s important to note that service initialization differs significantly between dev and local environments, so introducing a new service that operates in both contexts is the best solution.
Add reverse proxy configuration (local environment) f8ae77a
Added a new Caddy rule in the LMS to route all requests under PARAGON_STATIC_URL_PREFIX to the NGINX service, ensuring that only the minified files are served.
Edit development and production settings for the lms service 495a875
Updated the
MFE_CONFIGsetting to include thePARAGON_THEME_URLSobject based onPARAGON_ENABLED_THEMES, with URLs generated dynamically for each environment (local/dev). Please Please note that in dev environments the Caddy service is no longer available, and the base URL for the themes becomeshttp://localhost:<exposed_port>.Note
Documentation updates, testing, and Kubernetes environment integration will be delivered in separate PRs to simplify reviews.