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

docs: tweak webpack config design in assets ADR #31949

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Mar 16, 2023

Changes

Drop print_asset_settings in favor of cleaning up Django settings

@feanil @jmbowman , this revisits a decision we came to on the PR that introduced the assets rewrite ADR: #31790 (comment) . As I've worked on implementing it, I've come to feel that a print_assets_settings command would be tech debt as soon as we added it. Consider the three Django setting it'd need to print:

  • JS_ENV_EXTRA_CONFIG: This is a JSON dictionary. Taking it from a Django setting and then escaping & piping it through a management command into an environment variable has always been crazy, especially since it's not used by any Python code. This ought to just be a build-time environment variable for Webpack to consume. Tutor has always just ignored this Django setting, and I'm told (edge.)edx.org doesn't override it, so why not take this opportunity to kill it?
  • WEBPACK_CONFIG_PATH: This is also silly to have as a Django setting, as it is unused by Python code. Again, Tutor has never respected this Django setting, and I don't recall (edge.)edx.org using it either, so I recommend killing it too.
  • That just leaves STATIC_ROOT. Do we really want a special management command just to print out the static root? I propose we just use the existing print_setting command for it.

Explain OPENEDX_BUILD_ASSETS_OPTS environment variable

This was always part the plan in my head, but in order to explain the changes above, I've spelled it out in the ADR.

Add 'non-Tutor migration guide'

This just points up to the Reimplementation Specification section of the ADR. Nothing new; just wanted to make it extra where non-Tutor deployers should look for migration instructions.

Latest rendered ADR

https://github.com/kdmccormick/edx-platform/blob/kdmccormick/assets-adr-update/docs/decisions/0017-reimplement-asset-processing.rst

Post-merge

@kdmccormick kdmccormick force-pushed the kdmccormick/assets-adr-update branch 2 times, most recently from a89911d to 9e1fc18 Compare March 16, 2023 17:29
@kdmccormick kdmccormick changed the title docs: update assets ADR based on learnings from implementation docs: tweak webpack config design in assets ADR Mar 16, 2023
@kdmccormick kdmccormick force-pushed the kdmccormick/assets-adr-update branch from 9e1fc18 to dac54fe Compare March 16, 2023 18:10
@kdmccormick kdmccormick marked this pull request as ready for review March 16, 2023 18:19
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable cleanup to do at this time, thanks for digging into it.

@jmbowman
Copy link
Contributor

There's at least one recent usage of JS_ENV_EXTRA_CONFIG in #31313 , probably worth reaching out to see if there's an alternate approach and/or more use cases for customization that we've overlooked.

I think the main use case of WEBPACK_CONFIG_PATH is to automatically pick between dev or prod webpack settings depending on how we're running the service (which Django settings file); do you envision any way of picking this automatically without the variable, or will developers need to be aware of the environment variable and its options whenever building static assets? Just the existence of this dichotomy suggests that we may need to branch between Docker images for development vs production before building static assets.

Other than that, looks ok at a glance.

@kdmccormick
Copy link
Member Author

There's at least one recent usage of JS_ENV_EXTRA_CONFIG in #31313 , probably worth reaching out to see if there's an alternate approach and/or more use cases for customization that we've overlooked.

Good catch @jmbowman , I'll reach out just to make sure that defining JS_ENV_EXTRA_CONFIG as a build-time environment will work them.

Just the existence of this dichotomy suggests that we may need to branch between Docker images for development vs production before building static assets.

Yup, that's what Tutor does. Webpack is run with prod config to build the base (production) image, and then Webpack is re-run with dev config in order to build the development image.

do you envision any way of picking this automatically without the variable, or will developers need to be aware of the environment variable and its options whenever building static assets?

At least in Tutor, the OPENEDX_BUILD_ASSETS_OPTS environment variable will be set so that a bare invocation of scripts/build-assets.sh chooses prod or dev appropriately, with devs needing to think about it. Devstack or other distributions should be able to do the same.

@kdmccormick kdmccormick merged commit 463b64d into openedx:master Mar 16, 2023
@kdmccormick kdmccormick deleted the kdmccormick/assets-adr-update branch March 16, 2023 20:25
@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.

# but use /openedx/staticfiles as the static root:
scripts/build-assets.sh --theme-dirs ./mythemes

Furthermore, to facilitate a Python-free build reimplementation, we will remove two Django settings related to assets. These settings have never worked in Tutor, and 2U states that edx.org does not use them. However, on the off chance that some community operators rely on them, there exist alternative configuration methods for each, which will work both with and without Tutor:
Copy link
Member Author

@kdmccormick kdmccormick Mar 16, 2023

Choose a reason for hiding this comment

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

However, on the off chance that some community operators rely on them

Whoops, I should have reworded this, since Jeremy pointed out that OpenCraft probably uses JS_ENV_EXTRA_CONFIG. Anyway, I reached out in #opencraft to make sure they see this.

@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.

4 participants