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

Use proper npm version to run app tests #267

Merged
merged 1 commit into from
Jul 5, 2023

Conversation

NoelDeMartin
Copy link
Contributor

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
Copy link
Contributor

scara commented Jul 3, 2023

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: moodlehq/moodleapp@2a61ff4.
Should we also introduce ./patches here?

TIA,
Matteo

@NoelDeMartin
Copy link
Contributor Author

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
Copy link
Contributor

NeillM commented Jul 4, 2023

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
Copy link
Member

stronk7 commented Jul 4, 2023

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 stronk7 merged commit 9942d10 into moodlehq:master Jul 5, 2023
@stronk7
Copy link
Member

stronk7 commented Jul 5, 2023

Sold! Thanks!

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