feat!: remove dependency on Paver scripts #1042
Conversation
|
@regisb here it is! |
|
I did some more local testing and now I'm hitting this error 🤦🏻 Sorry, I guess this PR isn't quite ready to go yet. @regisb , would you be willing to give it a preliminary review, so that Dawoud and I could feel feel comfortable merging it while you're away? |
| @@ -1,218 +0,0 @@ | |||
| #! /usr/bin/env python | |||
| Here is a migration guide, where each command is to be run in the `lms` or `cms` container. | ||
| | **Before** | **After** | | ||
| |------------------------------------------|-------------------------------------------------------------------------------------| | ||
| | `openedx-assets build --env=prod ARGS` | `npm run build -- ARGS` | |
There was a problem hiding this comment.
This is very clear for people who are familiar with Tutor's openedx-assets comand. Is there a reference document that explains all these new npm commands? Unfortunately we can't add comments to package.json because it's JSON.
There was a problem hiding this comment.
There isn't yet, but I'll add a reference doc to edx-platform and link it here.
There was a problem hiding this comment.
kdmccormick
force-pushed
the
kdmccormick/assets-build
branch
from
344f9a7 to
554176e
Compare
| COMPREHENSIVE_THEME_DIRS: ["/openedx/themes"] | ||
| STATIC_ROOT_BASE: "/openedx/staticfiles" |
There was a problem hiding this comment.
Note for reviewers: Since this edx-platform change, we can specify these as environment variables (in the Dockerfile) and they will be automatically loaded into Django settings.
kdmccormick
force-pushed
the
kdmccormick/assets-build
branch
from
554176e to
610135c
Compare
|
@DawoudSheraz , could you review this? |
|
The image has been built successfully. But the indigo theme hasn't been applied to openedx(LMS, CMS) and it has been applied to MEFs perfectly. Getting this error in LMS container while I've checked the indigo theme is mounted correctly to LMS container. I am debugging this why I've got this error. [openedx.core.djangoapps.theming.helpers] [user None] [ip 192.168.65.1] helpers.py:209 - Theme not found in any of the themes dirs. [Theme 'indigo' not found in any of the following themes dirs,
2024-05-09 19:54:28 Theme dirs:
2024-05-09 19:54:28 []]
2024-05-09 19:54:28 Traceback (most recent call last):
2024-05-09 19:54:28 File "/openedx/edx-platform/./openedx/core/djangoapps/theming/helpers.py", line 204, in get_current_theme
2024-05-09 19:54:28 themes_base_dir=get_theme_base_dir(site_theme.theme_dir_name),
2024-05-09 19:54:28 File "/openedx/edx-platform/./openedx/core/djangoapps/theming/helpers.py", line 242, in get_theme_base_dir
2024-05-09 19:54:28 raise ValueError(
2024-05-09 19:54:28 ValueError: Theme 'indigo' not found in any of the following themes dirs,
2024-05-09 19:54:28 Theme dirs:
2024-05-09 19:54:28 [] |
| @@ -5,9 +5,6 @@ | |||
| from openedx.core.lib.derived import derive_settings | |||
|
|
|||
| ENABLE_COMPREHENSIVE_THEMING = True | |||
| COMPREHENSIVE_THEME_DIRS.append('/openedx/themes') | |||
There was a problem hiding this comment.
On my system, ENABLE_COMPREHENSIVE_THEMING is already enabled and have confirmed this using shell of lms container. and in ENV variables, COMPREHENSIVE_THEME_DIRS is added. the problem is LMS doesn't pick COMPREHENSIVE_THEME_DIRS from Env variables. It picks its value using themes_dirs = get_theme_base_dirs_from_settings(settings.COMPREHENSIVE_THEME_DIRS).
There are two options here:
- move
COMPREHENSIVE_THEME_DIRSback to settings variable - Make changes in edx-platform to pick
COMPREHENSIVE_THEME_DIRSvalue usingget_env_setting
Any of these solution is not being done in https://github.com/openedx/edx-platform/pull/34318/files.
If there seems other alternative solution, feel free to propose here.
There was a problem hiding this comment.
Thank you for testing @hinakhadim . Are you sure you are on the latest master for edx-platform? It's possible I missed something, but my intention was to load settings.COMPREHENSIVE_THEME_DIRS from the environment variable of the same name: https://github.com/openedx/edx-platform/blob/0d4adaa5d70364fe9ff0857b15cbc58cf952f584/cms/envs/common.py#L2205
COMPREHENSIVE_THEME_DIRS = os.environ.get("COMPREHENSIVE_THEME_DIRS", "").split(":")
This behavior was added in this commit openedx/edx-platform@21a1235 (original PR)
There was a problem hiding this comment.
Thanks for information, @kdmccormick. I have tested with latest master for edx-platform and it is now loading the indigo theme and picking value of COMPREHENSIVE_THEME_DIRS from env. So, everything is fine from my side.
BREAKING CHANGE: `openedx-assets` is replaed with `npm run` subcommands. For details, see the changelog entry. For further details and rationale, see the upstream DEPR ticket: openedx/edx-platform#31895
kdmccormick
force-pushed
the
kdmccormick/assets-build
branch
from
610135c to
f00c311
Compare
|
@DawoudSheraz @hinakhadim did either of you want folks want more time to review before I merge this? |
I have not had a chance to test it out locally. Although, I trust Hina's testing and insights on this. |
|
@kdmccormick Can you please elaborate this? https://github.com/openedx/edx-platform/blob/master/scripts/watch_sass.sh#L96 . I tried to run |
|
@hinakhadim you're absolutely right, that is my mistake! Could you review the fix openedx/edx-platform#34805 ? I will also backport that to Redwood. |
|
thanks @kdmccormick for the fix. I have tested with |
|
Thanks @hinakhadim ! |
Description
feat!: remove dependency on Paver scripts
BREAKING CHANGE:
openedx-assetsis replaced withnpm runsubcommands. For details, see the changelog entry.For further details and rationale, see the upstream DEPR ticket: openedx/edx-platform#34467
Testing
I have been testing this locally with both the default theme and Indigo, both
localanddev.Since openedx/edx-platform#34318 merged, these
npmbuild commands are being used for production edx.org builds, and no issues have been identified so far.Future work
This is one of the few remaining steps before we can delete Paver from edx-platform. It will also unlock the ability to build assets without installing edx-platform's base Python requirements, which could seriously speed up the image build.
Merge deadline
In order to do the "future work" before Sumac, I'd like to merge this PR before Redwood (Jun 9). Ideally, we merge this before the testing period begins (May ~9) so that BTR has a chance to catch any issues.