Patch - Githubissues

YunoHost-Apps / facilmap_ynh

Facilmap package for YunoHost

https://facilmap.org/

GNU Affero General Public License v3.0

6 stars 3 forks source link

Patch #5

Closed ericgaspar closed 3 years ago

ericgaspar commented 3 years ago

Problem

Description of why you made this PR

Solution

And how do you fix that problem

PR Status

[ ] Code finished.
[ ] Tested with Package_check.
[ ] Fix or enhancement tested.
[ ] Upgrade from last version tested.
[ ] Can be reviewed and tested.

Package_check results

An automatic package_check will be launch at https://ci-apps-dev.yunohost.org/, when you add a specific comment to your Pull Request: "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!"*

ericgaspar commented 3 years ago

!testme

yunohost-bot commented 3 years ago

:stuck_out_tongue_winking_eye:

cdauth commented 3 years ago

I'm still new to YunoHost, so I might not understand all the mechanisms, but I'm wondering:

Why did you get rid of the reusable functions such as facilmap_create_db and facilmap_install_app and replaced them with the raw code that needs to be duplicated in the install and upgrade script?
Why did you change the ownership of the app directory from root to the app user?
Why are you suppressing error messages in the npm install command?

ericgaspar commented 3 years ago

I'm still new to YunoHost, so I might not understand all the mechanisms, but I'm wondering:
* Why did you get rid of the reusable functions such as `facilmap_create_db` and `facilmap_install_app` and replaced them with the raw code that needs to be duplicated in the `install` and `upgrade` script?

It makes sense to use functions to avoid copying the same code everywhere but we chose not to do that on the example_ynh template and we want to keep the packages as close to that template as possible.

* Why did you change the ownership of the app directory from `root` to the app user?

cf.example_ynh recommandations

* Why are you suppressing error messages in the `npm install` command?

It mainly cosmetic. However the CI linter checks warnings frequency. You can revert this part if you want the warnings back.

cdauth commented 3 years ago

To me the permissions seem like a bad idea. An app should not be able to write its own executable files. Otherwise if there is some bug in the app that allows writing files on the server, it can be used to execute arbitrary code on the server.

For the error messages, I understand the cosmetic reasons for it. But it seems to me that it would make things hard to debug if this command fails for some reason. So unless we can find a way how it would output the errors only in case the command fails, I would think it's a good idea to generally enable error messages again.