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: move collectstatic ignore patterns into configuration #31934

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Mar 15, 2023

Overview

We are reimplementing edx-platform's static asset processing system. The three main parts are:

  1. building
  2. collection (prod only)
  3. watching (dev only)

This PR addresses (2)

Description

Django provides the collectstatic management command, which collects static assets into the STATIC_ROOT so that they can be served by some system external to Django (like nginx or caddy), as is usually desired in production environments.

edx-platform contains several types of files that we don't want to be collected into the STATIC_ROOT. Previously, these files had to be supplied to the management command using the --ignore option:

./manage.py lms collectstatic --ignore geoip --ignore sass ...etc
./manage.py cms collectstatic --ignore geoip --ignore sass ...etc

This yields a long, hard-to-remember command. Paver wrapped the command in its big paver update_assets task, but that task also builds assets, which is totally overkill when you're just trying to collect them.

Fortunately, collectstatic's default ignore patterns can be configured by defining a custom AppConfig class. We define such an config (EdxPlatformStaticFilesConfig) for LMS and CMS. Now, devs can collect LMS & CMS assets with just:

./manage.py lms collectstatic
./manage.py cms collectstatic

Further reading on collecstatic and --ignore:
https://docs.djangoproject.com/en/3.2/ref/contrib/staticfiles/#customizing-the-ignored-pattern-list

Further reading on eschewing Paver:
https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst

Closes: #31658

Testing instructions

Here are steps I used to test. They assume you have Tutor Nightly installed.

  • Ensure your Tutor Nightly is up to date and running:
    • In your tutor repo, check out nightly and git-pull.
    • tutor config save
    • tutor images pull all
    • tutor local stop && tutor local start -d
  • Generate lists of static files from the LMS & CMS containers and save them to your machine:
    • tutor local exec lms find /openedx/staticfiles | sort > staticfiles-lms-master.out
    • tutor local exec cms find /openedx/staticfiles | sort > staticfiles-cms-master.out
  • Confirm both files have 29171 lines:
    • wc -l staticfiles-lms-master.out
    • wc -l staticfiles-cms-master.out
  • Rebuild the openedx image using this version of edx-platform:
    • tutor config save --set EDX_PLATFORM_REPOSITORY=https://github.com/kdmccormick/edx-platform --set EDX_PLATFORM_VERSION=kdmccormick/assets-collect
    • tutor images build openedx
    • tutor local stop && tutor local start -d
  • Re-generate lists of static files from the LMS & CMS containers and save them to your machine:
    • tutor local exec lms find /openedx/staticfiles | sort > staticfiles-lms-pr.out
    • tutor local exec cms find /openedx/staticfiles | sort > staticfiles-cms-pr.out
  • Confirm that static files lists for LMS and CMS match. These commands should either have no output, or should have a trivial output (for example, the hashes of some assets may have changed; this is OK as long as that for every files missing from one list, it is present with a similar filename in the other list)
    • diff staticfiles-lms-master.out staticfiles-lms-pr.out
    • diff staticfiles-cms-master.out staticfiles-cms-pr.out

@kdmccormick kdmccormick force-pushed the kdmccormick/assets-collect branch 3 times, most recently from 8efba7f to 26990f6 Compare April 18, 2023 19:42
@kdmccormick kdmccormick marked this pull request as ready for review April 18, 2023 20:07
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.

So simple! I love it.

Copy link
Contributor

@andrey-canon andrey-canon left a comment

Choose a reason for hiding this comment

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

I think this is a good improvement I just left two options that you could consider

pavelib/assets.py Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't affect anything, but if you include this in a folder/file like openedx/core/lib/staticfiles/apps.py this will be more djangoapps-style and give us the chance to extend or modify the standard behavior in other files in the same folder

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Since we'd then be treating staticfiles like a tiny Django app, how about openedx/core/djangoapps/staticfiles/apps.py (instead of /lib/)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is also a good location

Django provides the `collectstatic` management command, which collects static
assets into the STATIC_ROOT so that they can be served by some system external
to Django (like nginx or caddy), as is usually desired in production environments.

edx-platform contains several types of files that we don't want to be collected
into the STATIC_ROOT. Previously, these files had to be supplied to the manageme
nt command using the `--ignore` option:

    ./manage.py lms collectstatic --ignore geoip --ignore sass ...etc
    ./manage.py cms collectstatic --ignore geoip --ignore sass ...etc

This yields a long, hard-to-remember command. Paver wrapped the command in its
big `paver update_assets` task, but that task also builds assets, which is totally
overkill when you're just trying to collect them.

Fortunately, `collectstatic`'s default ignore patterns can be configured by
defining a custom AppConfig class. We define such an config
(`EdxPlatformStaticFilesConfig`) for LMS and CMS. Now, devs can collect LMS and
CMS assets with just:

    ./manage.py lms collectstatic
    ./manage.py cms collectstatic

Further reading on collecstatic and --ignore:
https://docs.djangoproject.com/en/3.2/ref/contrib/staticfiles/#customizing-the-i
gnored-pattern-list

Further reading on eschewing Paver:
https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplem
ent-asset-processing.rst

Closes: openedx#31658
instead of:
./manage.py [lms|cms] collectstatic --ignore geoip --ignore sass ... etc.
"""
name = 'openedx.core.djangoapps.staticfiles'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but I think the failed tests are due to this change, please consider to keep the original name since changing it can cause problems in other parts of the code that depend on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrey-canon I did end up reverting this line.

I believe tests were failing because putting 'openedx.core.djangoapps.staticfiles' in INSTALLED_APPS wasn't enough. I needed to specify the full path to the config class: 'openedx.core.djangoapps.staticfiles.apps.EdxPlatformStaticFilesConfig'.

I believe that's because the class isn't a direct subclass of AppConfig, so Django won't consider it the "default" app config class for our new staticfiles app.

@kdmccormick
Copy link
Member Author

Ready for another review pass @andrey-canon .

@kdmccormick kdmccormick merged commit 151c4fc into openedx:master Apr 21, 2023
@kdmccormick kdmccormick deleted the kdmccormick/assets-collect branch April 21, 2023 12:26
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

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.

Collect edx-platform assets without Paver
4 participants