-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add nvm installation to install step. #312
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #312 +/- ##
============================================
+ Coverage 88.25% 88.30% +0.04%
- Complexity 739 742 +3
============================================
Files 76 76
Lines 2274 2291 +17
============================================
+ Hits 2007 2023 +16
- Misses 267 268 +1 ☔ View full report in Codecov by Sentry. |
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.
Hi @kabalin, this looks good enough. I've just added a couple of details for your consideration.
And then there is the thing (discussed in the chat) about, in some of the tests, always remove any potential $HOME/.nvm
dir that may be there to ensure that things are installed (or skipped) properly.
Feel free to merge it once amended, 100%.
One last suggestion, maybe I'd try, with some real plugin, both using 22.04 and 24.04 and verify that it's working in both cases. Sure (99%) it does, but trying with a real plugin is reassuring, IMO.
Ciao :-)
Install 0.39.7 explicitly as part of vendor installation, so we have control over the version we use. For cases where nvm is not required (pre-installed in the image, use --no-nvm param for install command) Fixes moodlehq#309
Converting this to draft due to ubuntu 24.04 incompatibility (different default installation dir), we might get back to this in future. |
Install 0.39.7 explicitly as part of vendor installation, so we have control over the version we use.
For cases where nvm is not required (pre-installed in the image, use --no-nvm param for install command)
Fixes #309