docs: add ADR: "Reimplement edx-platform static asset processing" - #31790
Merged
Conversation
kdmccormick
changed the title
kdmccormick
changed the title
davidjoy
reviewed
Feb 17, 2023
davidjoy
approved these changes
Feb 17, 2023
davidjoy
left a comment
davidjoy
left a comment
Contributor
There was a problem hiding this comment.
This seems like a great step forward toward more standards-based/vanilla asset processing... where I consider bash much more of a 'standard' than paver. 😂
One small comment on a command name, but otherwise in general this feels like we'll be in a better place after this work is done.
jmbowman
reviewed
Feb 17, 2023
jmbowman
left a comment
jmbowman
left a comment
Contributor
There was a problem hiding this comment.
Looks pretty good, just a few suggestions.
kdmccormick
commented
Feb 20, 2023
kdmccormick
left a comment
kdmccormick
left a comment
Member
Author
There was a problem hiding this comment.
Question for reviewers:
This was referenced Feb 21, 2023
1 task
feanil
approved these changes
Feb 21, 2023
feanil
left a comment
feanil
left a comment
Contributor
There was a problem hiding this comment.
The general approach looks good to me. I made a few suggestions for details that I think should be made, but I don't need to look at it again before it merges unless there are major changes to the approach.
regisb
reviewed
Mar 2, 2023
Co-authored-by: Feanil Patel <feanil@tcril.org>
Member
Author
Contributor
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
Contributor
|
EdX Release Notice: This PR has been deployed to the production environment. |
This was referenced Mar 12, 2023
kdmccormick
added a commit
that referenced
this pull request
Mar 16, 2023
Follow-up from: #31790 (comment) Part of DEPR: #31895
kdmccormick
added a commit
that referenced
this pull request
Jul 17, 2023
…32717) During the review of ADR 17 [1], Régis pointed out [2] that the shell script which replaces Paver's `process_npm_assets` could be automatically invoked as an NPM post-install hook, ensuring that the step is seamlessly executed whenever `npm install` is run. I had avoided using that suggestion, as I worried that it would make it harder to move node_modules out of the edx-platform directory in Tutor's openedx image. Since then, two things have changed. Firstly, Tutor v16's new persistent mounts interface [3] has lessened the importance of moving node_modules. Secondly, I have realized that using a post-install hook would not preclude us from modifying the underlying script (scripts/copy-node-modules.sh) to look in an alternative location for node_modules, should that end up being something we want to do. This commit modifies the ADR based on those findings, stubs out Paver's `process_npm_assets`, and adds the suggested post-install hook and replacement Bash script. References: 1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst 2. #31790 (comment) 3. https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14 Part of: #31604
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See included ADR for details. Link to latest rendered version of ADR.
Parent issue for all work related to implementing this ADR:
Draft PR of a working build.sh implementation, for reference:
Key reviewers: