YunoHost-Apps / paheko_ynh

Paheko package for YunoHost
https://paheko.cloud
GNU Affero General Public License v3.0
3 stars 5 forks source link

Keep config #50

Closed rodinux closed 9 months ago

rodinux commented 9 months ago

Problem

Solution

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)

rodinux commented 9 months ago

@alexAubin @ericgaspar , do you think we can do this ? Or is better add a file config.local.user.php dedicated to personalization on the folder __DATA__DIR__/data ??

I am just afraid about if someone try upgrade from older versions where we did not add all the new constants... in that case I would like to add something as

if ynh_compare_current_package_version --comparison lt --version 1.3.1~ynh1
then
   ynh_script_progression --message="Updating a configuration file..." --weight=5

  ynh_add_config --template="config.local.php" --destination="$install_dir/config.local.php"
  chmod 650 "$install_dir/config.local.php"
  chown $app:$app "$install_dir/config.local.php"
fi

Then, it can be a better way keep always the config.local.php without need bring it back from the config if admin users add personalization on the constants...

alexAubin commented 9 months ago

There is no straightforward way to address the issue, this is a shortcoming of the current strategy in packaging regarding app config 1) One the one hand, you want the new version of the app config to be applied in case there are new keys to be added etc ... 2) On the other hand, you don't want to overwrite manually modified files because the amin may have tweaked stuff

The "real" way to address this would be to keep the original config file somewhere and use like a git merge thing somehow, but for now we don't have the logistic for this

rodinux commented 9 months ago

There is no straightforward way to address the issue, this is a shortcoming of the current strategy in packaging regarding app config

1. One the one hand, you want the new version of the app config to be applied in case there are new keys to be added etc ...

2. On the other hand, you don't want to overwrite manually modified files because the amin may have tweaked stuff

The "real" way to address this would be to keep the original config file somewhere and use like a git merge thing somehow, but for now we don't have the logistic for this

Ok, what about doing like this ? I added a file config here $data_dir/data/config.local.user.php Then a line at the bottom of the file $intall_dir/config.local.php

/**
 * Chemin vers le fichier pour des configurations personnelles
 * qui ne sera pas écraser lors des mises à jour.
 */
require '__DATA__DIR__/data/config.local.user.php';

Like this we should keep the config.local.php safe and users can customize on the file in their $data_dir/data/config.local.user.php... For example, they could configure API key and user, isn't it ? or Nextcloud sync or wopi_url to use collabora or only_office...

rodinux commented 9 months ago

Ok, I found how makes it working... !testme

yunohost-bot commented 9 months ago

:stuck_out_tongue_winking_eye: Test Badge

rodinux commented 9 months ago

!testme

yunohost-bot commented 9 months ago

Fingers crossed! Test Badge

rodinux commented 9 months ago

Ok, So I have added some stuff when try

https://github.com/YunoHost-Apps/paheko_ynh/blob/9c0aa9a58481d9a23316fd1d1aee68a6fa0e474a/scripts/upgrade#L35-L44

https://github.com/YunoHost-Apps/paheko_ynh/blob/9c0aa9a58481d9a23316fd1d1aee68a6fa0e474a/scripts/upgrade#L109-L112

This works nice...

rodinux commented 9 months ago

!testme

yunohost-bot commented 9 months ago

:sunflower: Test Badge

rodinux commented 9 months ago

!testme

yunohost-bot commented 9 months ago

:rocket: Test Badge

rodinux commented 9 months ago

!testme

yunohost-bot commented 9 months ago

May the CI gods be with you! Test Badge

rodinux commented 9 months ago

For me it's OK this time, after doing few tests...

rodinux commented 9 months ago

!testme

yunohost-bot commented 9 months ago

Alrighty! Test Badge

rodinux commented 9 months ago

There is no straightforward way to address the issue, this is a shortcoming of the current strategy in packaging regarding app config

1. One the one hand, you want the new version of the app config to be applied in case there are new keys to be added etc ...

2. On the other hand, you don't want to overwrite manually modified files because the amin may have tweaked stuff

The "real" way to address this would be to keep the original config file somewhere and use like a git merge thing somehow, but for now we don't have the logistic for this

Hello, after lot of tests, I think I can push this PR, it will keep a config.local.user.php for the users in the $data_dir/data/ folder, needed for add some personal configurations and keep them on an upgrade. It keep the original config file also.

Do you approve ? also @ericgaspar is it OK for you ?

ericgaspar commented 9 months ago

approved

but to be honest I am not really following the package :)

rodinux commented 9 months ago

but to be honest I am not really following the package :)

Ok thanks.