-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use the new edx-platform assets build + other improvements #34
Conversation
BuildKit replaces and improves the legacy Docker builder, which was deprecated back in Feb 2023. Assuming BuildKit allows us to simplify the Dockerfile and makes future build performance improvements easier. The Docker versions which Tutor recommends (v20+) all come with BuildKit, so As follow-up work, we will need to remove `is_buildkit_enabled` from the official plugins templates. Relevant discussion: overhangio#868 (comment)
TODOs: * See if we can reduce the huge mount blocks a bit. * Merge edx-platform asset folder changes instead of patching in a PR. * Add changelog entry. * Test more thoroughly. * Circulate a TEP or some other form of proposal? * Deprecate patches that no longer exist or have changed. Part of: https://github.com/openedx/wg-developer-experience/issues/166
TODO: * Changelog entry * Comments * Linting * Tests * TEP
type=click.Path(dir_okay=True, file_okay=False, resolve_path=True), | ||
) | ||
@click.pass_obj | ||
def copyartifacts(context: BaseComposeContext, mount_paths: list[Path]) -> None: |
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.
I'm wondering if it makes sense to provide a dedicated command. In which cases will users have to run it? If I understand correctly, they will only run it when they don't want to run launch
. But then this opens up the possibility that they will forget to build the Docker image before running copyartifacts
. In such a scenario, the artifacts will be outdated, right?
I might not 100% understand the pros and cons, but it seems to me that it would be more intuitive if artifacts are copied as part of do init
.
COPY --link --from=edx-platform /openedx/features/course_search/static openedx/features/course_search/static | ||
COPY --link --from=edx-platform /openedx/features/learner_profile/static openedx/features/learner_profile/static | ||
COPY --link --from=edx-platform /xmodule/assets xmodule/assets | ||
COPY --link --from=edx-platform /xmodule/js xmodule/js |
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.
Wooooooow... Are all "static" directories copied? Does it mean that every new edx-platform app will require a change in Tutor? If yes, that's an issue.
Same comment below for sass directories.
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.
Wooooooow... Are all "static" directories copied?
...yes 😬
Does it mean that every new edx-platform app will require a change in Tutor?
In theory no, since apps should not be adding static assets to edx-platform; all new frontends should be MFEs.
That said, upstream refactorings or deprecations could definitely break this build stage. I agree it has far too many COPY statements.
I would still very much like to keep it so that they static asset build isn't re-run every time backend edx-platform code is changed. We need a way to get all of our frontend sources into a build stage without also copying backend assets, but we want to avoid hard-coding a big list of static directories.
I did some research, and here's what I came up with: #35. Let me know what you think.
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.
Yes, #35 looks much more tractable.
COPY --link --chown=app:app --from=bundles-production /openedx/edx-platform/common/static/bundles common/static/bundles | ||
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/lms/static/css lms/static/css | ||
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/lms/static/certificates/css lms/static/certificates/css | ||
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/cms/static/css cms/static/css |
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.
Again, this long list leads me to believe that it's prone to errors.
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.
While I agree with your other comment, this particular list is solidified. It's long because edx-platform is complicated, but it's not going to grow. Breaking it up a bit, it's:
# Copy in collected staticfiles. These are generally just links to the actual static assets, copied in below.
COPY --link --chown=app:app --from=bundles-production /openedx/staticfiles /openedx/staticfiles
# Copy in Webpack-generated JS bundles for both LMS and CMS.
COPY --link --chown=app:app --from=bundles-production /openedx/edx-platform/common/static/bundles common/static/bundles
# Copy in Sass-compiled CSS for LMS.
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/lms/static/css lms/static/css
# Copy in Sass-compiled CSS for HTML certificates.
# (Certificates are meant to be displayable outside of the context of LMS, so edx-platform
# organizes their static assets into a distinct subfolder).
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/lms/static/certificates/css lms/static/certificates/css
# Copy in Sass-compiled CSS for CMS.
COPY --link --chown=app:app --from=css-production /openedx/edx-platform/cms/static/css cms/static/css
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.
OK, I trust your judgment here.
copying in the dev virtualenv was clobbering the symlink which `pip install -e .` creates in the virtualenv. fixes this error in dev mode: Traceback (most recent call last): File "/openedx/bin/site-configuration", line 3, in <module> import lms.startup ModuleNotFoundError: No module named 'lms'
Running `tutor config save` with an outdated version of pycryptodome was failing with the following error: Error: Missing configuration value: 'Crypto.PublicKey.RSA.RsaKey object' has no attribute 'dq' This is because the "dq" attribute was only introduced in pycryptodome 3.17.0: https://www.pycryptodome.org/src/changelog#january-2023 To resolve this issue we bump the minimum requirements. Close overhangio#962
This test fixture has been happily living in the test folder for 4 years...
Environment was not updated correctly on `tutor plugins enable ...` because of a caching issue. To bypass this issue, we improve the caching mechanism and introduce a new `lru_cache` decorator. Close overhangio#989.
We somehow forgot to include these files in a previous commit...
0c8a126
to
020787e
Compare
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
…dmccormick/assets
It's a 50MB optimization, which is nice, but not worth these Dockerfile lines. This optimizatoin will happen naturally in Sumac, anyway, since Paver's deps will be removed from base.txt
fixes permission error
This branch is superseded by:
Improvements
npm run build
),COPY --link
andCOPY --from=<mounted-build-context>
,development
isn't based onproduction
. Rather, they are now both based onapplication
.tutor ... copyartifacts
is availble to copy pre-built artifacts from the image into bind-mounted repos.tutor ... launch
no longer needs to re-install node_modules or rebuild assets; it just runstutor ... copyartifacts
as part of setup.How to use this with a bind-mounted edx-platform repo
You need a recent version of edx-platform master. It must include openedx/edx-platform@db45976 (merged 2023-08-31).
Re-run first-time setup. Steps are unchanged, but
launch
should feel noticeably quicker:The initial image build will be slower since the new Dockerfile will build mostly from scratch, but subsequent image builds should feel quick and lightweight.
Furthermore, in the past, if you wanted to rebuild static assets in your bind-mounted edx-platform (either to test asset changes, or just because your assets have fallen out-of-date), you'd have to re-run these, which could take several minutes:
Instead, you can now just rebuild your image:
and copy its build artifacts (which will include node modules and static assets) down into your edx-platform:
This branch...
...is relatively stable by now. I will push tweaks and merge in
nightly
from time to time, but I won't force-push.