search

YunoHost-Apps / grav_ynh

Grav, a flat-file CMS packaged for YunoHost
https://getgrav.org/
GNU General Public License v3.0
16 stars 12 forks source link

Actually disable in-app backup, do not backup backups nor cache #163

Closed tituspijean closed 2 months ago

tituspijean commented 2 months ago

Fixes #162

PR Status

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

tituspijean commented 2 months ago

!testme

yunohost-bot commented 2 months ago

:stuck_out_tongue_winking_eye: Test Badge

yunohost-bot commented 2 months ago

:book: Test Badge

eauchat commented 2 months ago

Just had a review at the code, seems great to me. I was just wondering if conf/scheduler.yaml should not contain instead:

status:
  cache-purge: enabled
  cache-clear: enabled
  default-site-backup: disabled

Don't know what you think of it, since cache cleanup maybe is a good thing to have by default.

Also, I was wondering about the permissions used. Shouldn't the group be set to www-data as is done globally in the install script?

tituspijean commented 2 months ago

Don't know what you think of it, since cache cleanup maybe is a good thing to have by default.

Indeed, though it may increase load on lower spec devices?

Also, I was wondering about the permissions used. Shouldn't the group be set to www-data as is done globally in the install script?

Actually the PHP pool runs as $app:$app so there is no influence on that here. I'm actually wondering now about the relevance of www-data... ha yes, a look to nginx.conf makes sense of its use.

tituspijean commented 2 months ago

Also, the relevant part of the install script is actually https://github.com/YunoHost-Apps/grav_ynh/blob/68edebace10aabac9ccaa171353f522d54c0d92c/scripts/install#L63 ;)

Though I concede mixing $app:www-data and $app:$app files may not make actual sense.

eauchat commented 2 months ago

Yes, for coherence I'd have used the same permissions everywhere, but I've no idea what's better in fact.

tituspijean commented 2 months ago

Let's merge this as-is. I do not want to give myself a headache with files permissions right now. :)