-
Notifications
You must be signed in to change notification settings - Fork 32
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
Implement support for the installation/uninstallation of npm packages #5092
Implement support for the installation/uninstallation of npm packages #5092
Conversation
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.
GJ, but some small changes are required
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.
Good job, Just a few minor adjustments are needed.
It's advisable to create new methods named install_npm_package and remove_npm_package. Doing so will enhance the maintainability, testability, and readability of the code. This change will make it necessary to perform minor changes in the remote_operation module to handle which method is going to be used for each package
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.
Include this change in the changelog
Co-authored-by: Víctor Rebollo Pérez <victorrebollop@gmail.com>
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.
Install package and install npm packages should be totally different methods.
Currently install_package is too complex and it is difficult to maintain
@Rebits Modified here: 5e8d1ea and check in this comment: #5130 (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
Description
This PR implements the support to have the option to install and uninstall packages with npm on all endpoints.