search

moodlehq / moodle-docker

A docker environment for moodle developers
GNU General Public License v3.0
373 stars 244 forks source link

Use proper npm version to run app tests #267

Closed NoelDeMartin closed 1 year ago

NoelDeMartin commented 1 year ago

Following up from the discussion in #263, I've updated the app setup to use the proper npm version.

It seems like the errors were caused from a known issue in a library we're using to patch 3rd party dependencies, called patch-package.

This problem could technically be happening when using moodle-docker locally as well, not just in the CI environment. But nobody in our team has had any problems with it so far, and given the security implications of adding the unsafe-perm to the app repo, I prefer to fix it just on CI.

scara commented 1 year ago

Hi @NoelDeMartin, forgive my ignorance since I'm quite new to NPM: reading https://www.vinayraghu.com/blog/npm-unsafe-perm it looks like that config flag is no longer implemented/required starting from v8 i.e. just to mention it here, when the code will bump the node version (now 11 i.e. npm v6).

I've already read that usage within MOBILE-2314: https://github.com/moodlehq/moodleapp/pull/3556/commits/2a61ff495705325f64648f899cb5010e1f562300. Should we also introduce ./patches here?

TIA, Matteo

NoelDeMartin commented 1 year ago

Thanks for taking a look @scara,

I wasn't aware that this has been removed, so I'll keep that in mind when we update our node version.

About the patches folder, it is already included here because we're mounting the entire app folder. In the Dockerfile you linked we need to do it manually because we're only copying the package*.json files into the container.

NeillM commented 1 year ago

If I'm reading the docs correctly we are running into the issue because things are probably being run by root, on a directory owned by root (at least within Github actions).

I imagine the app developers have not had the same issue in their own local builds because you tend not to do that 😄

stronk7 commented 1 year ago

It's certainly all Chinese for me, so... if you all are happy with the change... I'm happy too. Please, thumbs-up to this comment or argue if there is anything to argue. I'll merge this once I get a +3 (or a +2 without any arguing in 24h).

Ciao :-)

stronk7 commented 1 year ago

Sold! Thanks!