build: copy from node_modules using NPM postinstall hook, not Paver - #32717
Merged
kdmccormick merged 1 commit intoJul 17, 2023
Merged
Conversation
kdmccormick
marked this pull request as ready for review
July 11, 2023 16:08
Member
Author
|
@regisb Would you remind reviewing this one? |
This was referenced Jul 11, 2023
davidjoy
approved these changes
Jul 11, 2023
regisb
approved these changes
Jul 13, 2023
regisb
left a comment
Contributor
There was a problem hiding this comment.
I did not test the change but I think I understand what is going on :) Looks good to me! I appreciate the fact that you made node_modules future-proof with an ad-hoc parameter.
kdmccormick
force-pushed
the
kdmccormick/copy-node-modules
branch
from
July 14, 2023 18:52
71bebff to
40d5ec3
Compare
TODO will fill in details from PR description Part of: openedx#31604
kdmccormick
force-pushed
the
kdmccormick/copy-node-modules
branch
from
July 17, 2023 11:48
40d5ec3 to
c1959a1
Compare
kdmccormick
added a commit
that referenced
this pull request
Jul 17, 2023
… Paver (#32766) Reverts #32717 since it is breaking the Docker build, both in the edx-platform CI, and for Tutor Nightly. edx@0.1.0 postinstall scripts/copy-node-modules.sh sh: 1: scripts/copy-node-modules.sh: not found The problems seems to be that `npm install` is run before `scripts/` is copied in, but the new post-install hook counts on `scripts/copy-node-modules.sh` existing. This reverts commit 4b64d83.
Contributor
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
Contributor
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
Contributor
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Contributor
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
Contributor
|
2U Release Notice: This PR has been deployed to the edX production environment. |
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
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.
During the review of ADR 17 [1], Régis pointed out [2] that the shell script which replaces Paver's
process_npm_assetscould be automatically invoked as an NPM post-install hook, ensuring that the step is seamlessly executed whenevernpm installis 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:
Part of: #31604
Testing
I tested this by comparing the exact node_modules asset copies, as generated by the old Paver command versus this new shell script & post-install hook.
Using latest Tutor Nightly and a recently-pulled openedx image: