-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add support for installing extra npm packages into edxapp #188
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
feat: add support for installing extra npm packages into edxapp #188
Conversation
syedimranhassan
left a comment
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.
LGTM
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.
nit:
| # --no-save is passed as a flag to npm install to avoid saving these dependencie to package.json. Otherwise, | |
| # --no-save is passed as a flag to npm install to avoid saving these dependencies to package.json. Otherwise, |
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.
[clarification] Does this item.version_range correspond to the documented options for EDXAPP_EXTRA_NPM_REQUIREMENTS (below)? Curious if the version and item.version_range keys need to match.
# List of additional npm packages that should be installed into the
# edxapp virtual environment.
# `name` (required), `version` (optional), and `extra_args` (optional)
# Example:
# EDXAPP_EXTRA_NPM_REQUIREMENTS:
# - name: mypackage
# version: ^1.0.0
# - name: git+https://git.myproject.org/MyProject#egg=MyProject
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.
Ah, yes. This should be version. Thanks for catching that!
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.
[question] Is this extra_args option used when installing these packages below?
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 agree that I didn't include them in the actual shell module, and I don't think it's necessary to support at the moment. I'll leave it off until we have a need for it.
f0ad949 to
44f70f5
Compare
| # EDXAPP_PRIVATE_NPM_REQUIREMENTS: | ||
| # - name: mypackage | ||
| # version: ^1.0.0 | ||
| # - name: git+https://git.myproject.org/MyProject#egg=MyProject |
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.
- How would you version a git reference? Also, can this and should this use a git tag or commit hash?
- We should update https://2u-internal.atlassian.net/wiki/spaces/AT/pages/396034066/How+to+add+private+requirements+to+edx-platform to have a Python section (current doc) and a new section for npm. This comment should reference the doc, whether the examples are moved or duplicated. This doc was referenced from the comment for EDXAPP_PRIVATE_REQUIREMENTS.
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.
Regarding your first comment, I believe that npm supports providing a semver version or exact version for installing from a git URL. Here's a link to the documentation. I can't hyperlink to the relevant section, but a search for npm install <git remote url> will get you there. npm supports installing from any git commit-ish. I don't have any feelings about whether we should support that.
It would be more simple to support a "name" variable that can contain any valid, installable reference an npm module, à la EDXAPP_PRIVATE_REQUIREMENTS. What do you think about that?
Regarding your second comment, I had plans to update the documentation you linked to, but I don't want to do that until this is approved and merged. I will update the comment to include a link to this documentation once we iron out the first comment, and then I'll update the documentation once this work is merged.
Thanks for your review!
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.
- Yes. I think having it work like EDXAPP_PRIVATE_REQUIREMENTS is fine, as long as we update examples, don't include examples with no version.
- I recommend moving the docs to the wiki page, and adding a link, and adding a simple section that contains the docs that are currently in this PR so we can update with different examples and keep everything together. That said, if you have your own plan for how and when you want to update the doc and links, and return to this again, that's up to you.
| # - name: mypackage | ||
| # version: ^1.0.0 | ||
| # - name: git+https://git.myproject.org/MyProject#egg=MyProject | ||
| EDXAPP_PRIVATE_NPM_REQUIREMENTS: [] |
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.
You don't have an actual npm requirement you wish to add yet? Is this all in preparation of some future change? Unfortunately, that makes it tougher to know if we are landing something that works.
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.
Yes, this is in preparation for the removal of the edx-proctoring-package from the edx-platform. Please see the DEPR issue.
I will be adding edx-proctoring-proctortrack to this list in the future, pending a conversation with Axim, but don't want to combine that change with this one. I'll put up a separate pull request later.
robrap
left a comment
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 don't really have any blocking comments, in case that isn't clear.
35ae3c8 to
d2506ce
Compare
This commit adds support for installing extra npm packages in edxapp to the edxapp deploy playbook. In order to add an extra npm package to edxapp, add the package to the EDXAPP_EXTRA_NPM_REQUIREMENTS list. For more help, see: https://2u-internal.atlassian.net/wiki/spaces/AT/pages/396034066/How+to+add+private+requirements+to+edx-platform.
8ff5b66 to
3884b14
Compare
Description
This commit adds support for installing extra npm packages in edxapp to the
edxappdeploy playbook. In order to add an extra npm package to edxapp, add the package to theEDXAPP_PRIVATE_NPM_REQUIREMENTSlist.For more help, please see: https://2u-internal.atlassian.net/wiki/spaces/AT/pages/396034066/How+to+add+private+requirements+to+edx-platform.
Jira: COSMO-683
Make sure that the following steps are done before merging: