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

build: Explicitly Install assets.txt in openedx image #1178

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kdmccormick
Copy link
Collaborator

From the upstream Paver DEPR [1], this accounts for a change in the requirements files:

Starting in Sumac, these dependencies will be removed from
requirements/edx/base.txt. Instead, operators will need to install:

  • requirements/edx/assets.txt to build static assets

In the future, we could optimize the openedx image build by installing assets.txt in a separate, rarely-invalidated cache layer. libsass in particular takes 60+ seconds to install, so this is promising. However, for now, in order to prepare for [1], we make just the simplest possible change, which is to install assets.txt along with base.txt.

We also take this opportunity to bump the nodeenv version from 1.8.0 to 1.9.1, matching edx-platform's. This is another area where we could make use of assets.txt in a future Dockerfile refactoring.

[1] openedx/edx-platform#34467

From the upstream Paver DEPR [1], this accounts for a change
in the requirements files:

> Starting in Sumac, these dependencies will be removed from
> requirements/edx/base.txt. Instead, operators will need to install:
> * requirements/edx/assets.txt to build static assets

In the future, we could optimize the openedx image build by installing
assets.txt in a separate, rarely-invalidated cache layer. libsass
in particular takes 60+ seconds to install, so this is promising.
However, for now, in order to prepare for [1], we make just the
simplest possible change, which is to install assets.txt along with
base.txt.

We also take this opportunity to bump the nodeenv version from 1.8.0 to
1.9.1, matching edx-platform's. This is another area where we could make
use of assets.txt in a future Dockerfile refactoring.

[1] openedx/edx-platform#34467
@kdmccormick
Copy link
Collaborator Author

@regisb I'm sure Sumac is your priority right now, but when you get a chance to look at it, this PR is the final barrier to us removing paver from edx-platform entirely.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

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

Yup, this looks good to me. But shouldn't we wait until Sumac is released? Such that we can rebase sumac on top of main if necessary. The paver removal is not compatible with Sumac, right?

@kdmccormick
Copy link
Collaborator Author

But shouldn't we wait until Sumac is released? Such that we can rebase sumac on top of main if necessary. The paver removal is not compatible with Sumac, right?

Sure, sounds good. I'll merge right after Sumac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants