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

[PROPOSAL] remove node_modules from ILIAS repository. #5128

Merged

Conversation

thibsy
Copy link
Contributor

@thibsy thibsy commented Oct 14, 2022

Hi folks,

In reaction upon @klees comment in #5114 I thought why not just remove the node_modules from the repository?

Reason alone could be my computer nearly crashing due to the huge diff of this PR alone. Same goes for the PR I just referenced above, which makes it really hard to figure out what the PR actually adds to the codebase.

It's also kind of inconsistent to include node_modules in the repository whereas the composer dependencies will have to be installed manually.

I believe I don't have to tell you about why this is a good thing :). If you strongly disagree of this though, please let me know.

Kind regards!

@thibsy thibsy changed the title [FEATURE] remove node_modules from ILIAS repository. [PROPOSAL] remove node_modules from ILIAS repository. Oct 14, 2022
@thibsy
Copy link
Contributor Author

thibsy commented Oct 14, 2022

P.S. I adjusted the documentation of the repository in install.md, if this PR should be merged the official documentation on the website needs to be updated as well.

@matthiaskunkel
Copy link
Member

Jour Fixe, 17 OCT 2022: We see several advantages of this PR but need to discuss with the BB if we can go this way and introduce another dependency (npm) that are a problem for some installations. One option would be to offer ready builts additionally.

@PurHur
Copy link
Contributor

PurHur commented Oct 18, 2022

Please have a really good look on the security side of this issue. NPM is not the yellow from the egg when it comes to security.

Adding
--ignore-scripts
to the docs should be done and only skipped if its really necessary. If a package needs this you should consider to replace it. Since the code of an npm package doesnt have to match e.g. a given Github Repo its nearly impossible to review the source correctly before deploying an update.

See:
https://cheatsheetseries.owasp.org/cheatsheets/NPM_Security_Cheat_Sheet.html

@thibsy
Copy link
Contributor Author

thibsy commented Oct 18, 2022

Thx @PurHur for drawing this to our attention. I updated the installation docs of the repository accordingly.

@kergomard
Copy link
Contributor

kergomard commented Oct 31, 2022

Thanks again @thibsy for taking the initiative for this! The Technical Board discussed your proposal at our meeting and we agree with the direction this takes. As this has the potential to complicate the installation and maintenance of ILIAS, we will need a little bit more time though, to come up with a good plan on how to provide a updates and installations in the future and how to communicate it well.

@Amstutz Amstutz removed their assignment Mar 2, 2023
@kergomard
Copy link
Contributor

Hi @thibsy
Could you please be so kind and resolve the conflicts once more? I would then merge this right away.
Thank you very much!
@kergomard

@thibsy
Copy link
Contributor Author

thibsy commented Mar 8, 2023

Hi @kergomard

This should be good to go now. Thx a lot!

Kind regards
@thibsy

@kergomard kergomard merged commit d892ba0 into ILIAS-eLearning:trunk Mar 9, 2023
@kergomard
Copy link
Contributor

Thank you very much @thibsy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants