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: copy from node_modules using NPM postinstall hook, not Paver #32717

Merged

Conversation

kdmccormick
Copy link
Member

@kdmccormick kdmccormick commented Jul 11, 2023

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. docs: add ADR: "Reimplement edx-platform static asset processing" #31790 (comment)
  3. https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14

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:

# ensure latest master & clean slate
cd edx-platform
git switch master
git pull
git clean -f common/static

# we'll use compare_common_static to hold the contents of common/static in a few different scenarios
rm -rf compare_common_static
mkdir compare_common_static

# scenario A: output from the current image
tutor local copyfrom lms common/static compare_common_static/image

tutor mounts add .

# scenario B: output from old paver task on master
git switch master
tutor local run lms openedx-assets npm
cp -r common/static compare_common_static/paver

# scenario C: output from new copy-node-modules shell script
git switch kdmccormick/copy-node-modules
tutor local run lms scripts/copy-node-modules.sh
tutor local copyfrom lms common/static compare_common_static/shell

# scenario D: output from new copy-node-modules shell script, invoked by npm post-install hook
git switch kdmccormick/copy-node-modules
tutor local run lms npm install
tutor local copyfrom lms common/static compare_common_static/npm-install

# compare out of all scenarios -- they should be the exact same! (diff should have no output)
cd compare_common_static
diff -r image paver
diff -r image shell
diff -r image npm-install

@kdmccormick kdmccormick marked this pull request as ready for review July 11, 2023 16:08
@kdmccormick kdmccormick requested a review from regisb July 11, 2023 16:09
@kdmccormick
Copy link
Member Author

@regisb Would you remind reviewing this one?

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.

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 kdmccormick force-pushed the kdmccormick/copy-node-modules branch from 71bebff to 40d5ec3 Compare July 14, 2023 18:52
TODO will fill in details from PR description

Part of: openedx#31604
@kdmccormick kdmccormick force-pushed the kdmccormick/copy-node-modules branch from 40d5ec3 to c1959a1 Compare July 17, 2023 11:48
@kdmccormick kdmccormick merged commit 4b64d83 into openedx:master Jul 17, 2023
@kdmccormick kdmccormick deleted the kdmccormick/copy-node-modules branch July 17, 2023 17:27
kdmccormick added a commit that referenced this pull request Jul 17, 2023
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.

    [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.
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX 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