-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: copy from node_modules using NPM postinstall hook, not Paver #32717
build: copy from node_modules using NPM postinstall hook, not Paver #32717
Conversation
@regisb Would you remind reviewing this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
71bebff
to
40d5ec3
Compare
TODO will fill in details from PR description Part of: openedx#31604
40d5ec3
to
c1959a1
Compare
… Paver (#32766) Reverts #32717 since it is breaking the Docker build, both in the edx-platform CI, and for Tutor Nightly. [email protected] 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.
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
2U Release Notice: This PR has been deployed to the edX production environment. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
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 whenevernpm 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:
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: