Skip to content
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

feat: upgrade to Palm #120

Closed
wants to merge 5 commits into from
Closed

feat: upgrade to Palm #120

wants to merge 5 commits into from

Conversation

regisb
Copy link
Contributor

@regisb regisb commented Apr 12, 2023

No description provided.

@regisb regisb requested a review from arbrandes April 12, 2023 08:42
@regisb regisb changed the title feat: upgrade to Palm draft: feat: upgrade to Palm Apr 12, 2023
@regisb regisb marked this pull request as draft April 12, 2023 08:43
@regisb regisb changed the title draft: feat: upgrade to Palm feat: upgrade to Palm Apr 12, 2023
@regisb regisb force-pushed the palm branch 2 times, most recently from f3815e9 to 9e9f94d Compare May 4, 2023 09:58
@regisb regisb marked this pull request as ready for review May 11, 2023 15:21
@regisb regisb force-pushed the palm branch 2 times, most recently from fadd940 to 9cdd6ef Compare May 16, 2023 13:05
FROM {{ app["name"] }}-common AS {{ app["name"] }}-prod
{%- for app_name, app in iter_mfes() %}
######## {{ app_name }} (production)
FROM {{ app_name }}-common AS {{ app_name }}-prod
ENV NODE_ENV=production
Copy link
Member

Choose a reason for hiding this comment

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

Cna you we add here support the new feuture
ARG USE_PURGECSS=false
ENV USE_PURGECESS=$USE_PURGECSS
So that it can set at build, i.e. docker build mfe --build-arg USE_PURGECESS=true
ref: https://github.com/openedx/frontend-build#optimization

Copy link
Contributor Author

@regisb regisb Jun 12, 2023

Choose a reason for hiding this comment

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

I just tested building the mfe image with USE_PURGECSS=true and now I think I know your pain when you build on macOS and Docker eats all your memory.

I'm reluctant to introduce a build-time argument for such a specific use case. Instead, can I suggest we add a {{ patch("mfe-dockerfile-prod-pre-build") }} statement?

EDIT: PurgeCSS reduced the CSS final size from 81kb to 28kb for the Account MFE. That's a huge relative improvement, but relatively small in absolute value, so I'm not sure it's worth the extra build time and resources -- not by default at least.

Copy link
Member

@ghassanmas ghassanmas Jun 14, 2023

Choose a reason for hiding this comment

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

Indeed I don't meant to have it by default, just to have it as env-variable would just it make it easier for people to use, it, an extra cli arg vs creating a plguin file. Secondaly regarding resources extensive #125, which is an issue even without using prugeCSS, https://discuss.openedx.org/t/npm-err-code-err-socket-timeout-when-building-mfe-image/10414. Lastly regarding the diff in file size, I would just add that it's not directly about the file size/request size, as it's about perfoamnce; the time it takes the browser to read compile the file content.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really prefer a simple plugin over a new build arg or tutor setting. The new patch would also have the advantage that it could also be used for other purposes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 for a new patch point.

regisb and others added 5 commits June 14, 2023 12:12
With this new release, we make use of persistent bind-mounts to make it
much easier to work on MFE forks. In addition, adding new MFEs is no
longer done with `*_MFE_APP` settings, which was awkward, but with
appropriate plugins.

Close #111.
The MFE is accessible by instructors in ORA exercises that have explicit staff
grading steps.

The corresponding waffle flag is installed and enabled by default.
The MFE is usable by instructors to send bulk email to learners in a course.

The feature itself (the ability to send bulk email) pre-dates this MFE, and
must be enabled as usual for the "Email" tab to become visible in the
Instructor Dashboard.  The difference is that with this change, the tab will
include a link to the MFE by default.
I realised that the hooks were imported in the wrong context when I ran
`tutor local do init --limit=ecommerce`. This caused the MFE init script
to run.
Copy link
Collaborator

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Totally approved. 👍🏼

@regisb regisb closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants