Magically keeping all custom-config panel settings during the conf upgrade via `ynh_add_config` without having to define all of them as yunohost settings

alexAubin commented 2 years ago

The problem

While studying config panels, I'm coming accross this one https://github.com/YunoHost-Apps/glitchsoc_ynh/blob/testing/config_panel.toml which basically expose ~10 settings from the app conf file to the config panel

The problem is that if all of these aren't declared as "actual" yunohost settings during install/upgrade, and used in the conf template, upgrading the conf file with ynh_add_config is gonna overwrite them during every app upgrade, which is a UX disaster

Having to handle a few additional settings during install/upgrade, but having to handle 10+ of these is boring as hell and symptomatic of how overly complex it is to expose settings in config panels

Possible solution

ynh_add_config could have a magic mecanic such that:

it would analyze config_panel.toml and identify entries binded to the target file
before the file is replaced, it could use ynh_read_var_in_file to fetch the corresponding value for foobar, and store it as $foobar (assuming $foobar) doesn't already exists ? (In fact this step and the previous one could be maybe easily handled by tweaking yunohost app config --export maybe ? Just need to somehow limit the export to settings bound to a specific file ?)
after the file is wrote/replaced, use ynh_write_var_in_file to write back the value (or maybe using yunohost app config --import ?)
(recompute and store the file hash)

This behavior could maybe be switched on/off by adding a property like keep_during_upgrade = true/false on each config panel setting, if that makes sense

Tagadda commented 2 years ago

I was thinking about config files that are not installed with ynh_add_config, or any other config_panel option that need to be re-applied after an upgrade.

First, we need AppConfigPanel._apply() to call super()._apply(), so each time a config is modified, we save the config item as an actual yunohost app settings.
And after each upgrade, call AppConfigPanel._call_config_script("apply") (which already reads app settings and apply the config with ynh_app_config_run). But this might not be a good solution? This would cause the service to restart once during the upgrade script without the config applied, then the config is applied and service restarted.

Gredin67 commented 1 year ago

I came across an issue that is related I think. When upgrading an app where:

    permissions:
        "__LISTRELAY__": relay
        "__LISTUSER__": user
        "__LISTADMIN__": admin

and listrelay are tags. I get

    permissions:
        "@user:domain.tld,@user2:domain.tld": admin
        "domain.tld,domain2.tld": user
        "*": relay

Thanks to, it should be:

    permissions:
        "@user:domain.tld": admin
        "@user2:domain.tld": admin
        "domain.tld": user
        "domain2.tld": user
        "*": relay

See e.g. https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/pull/74#issuecomment-1377201609

Can we run config_panel_apply on the config.yaml file after its initialization in upgrade?

Gredin67 commented 1 year ago

keep in mind this issue when refactoring https://github.com/YunoHost/issues/issues/2017

Tagadda commented 1 year ago

While creating a config_panel, I though about another approach to solve this issue. We could keep those config_panel's options as settings, and create a new "default" key in the config_panel.toml. This default key could be used to create app's settings during the provisioning step (before install and upgrade scripts).

jedie commented 9 months ago

With manifest v2 I'm currently have some duplicated information in "manifest.toml" and "config_panel.toml" like:

grafik

What's about to remove the "config_panel.toml" and store in "manifest.toml" the bind information and if this setting should be exposed on the config panel?

So you have one place for both and no duplicated "code"

alexAubin commented 9 months ago

What's about to remove the "config_panel.toml" and store in "manifest.toml" the bind information and if this setting should be exposed on the config panel?

Uuuh wokay but in the general case, some stuff in config panel are not install question and viceversa ... Also the config panel has the panel/section/option structure which is not present in the manifest

Nevertheless there's clearly a big topic between install questions, config panel, and how to initialize settings (e.g. random keys etc) and handle backward compat'

alexAubin commented 9 months ago

Yet another example today in https://github.com/YunoHost-Apps/fittrackee_ynh/pull/47 ...

zamentur commented 9 months ago

Solution 1: read values, upgrade the config and rewrite values

yunohost app config --export --related-to="$install_dir/config/config.php" > ./config.yml
ynh_add_config --template="../conf/config.php" --destination="$install_dir/config/config.php"
yunohost app config --import < ./config.yml

This solution avoid having to manage a lot of template vars in ../conf/config.php like __RATE_LIMIT__ however, it means the default values is defined in the template instead of __RATE_LIMIT__.

What if the config file (config.php) change of format ? Some keys inside could be renamed for example... I guess it could be a mess, cause when we run yunohost app config --export --related-to="$install_dir/config/config.php" the config_panel.toml is the new config_panel, but the config.php is the old one...
What if config_panel is corrupted or broken, should it broke the upgrade process ?

Draft: https://github.com/YunoHost/yunohost/pull/1675

Solution 2 : Save by default data in settings on config panel applying

Tag suggest to use super.apply(), but i noticed that values are often already registered in settings. https://github.com/YunoHost/yunohost/blob/dev/helpers/config#L123

We could discuss of the interest to save in settings when we use custom setter (or even file type).

But the problem with this approach is that the config panel could never be run by the user. And we can't simply apply it before to make the ynh_add_config in install script...

Solution 3: `default` key

We could keep those config_panel's options as settings, and create a new "default" key in the config_panel.toml. This default key could be used to create app's settings during the provisioning step (before install and upgrade scripts).

default keys already exists in ConfigPanel but not really in AppConfigPanel due to this mess about ynh_add_config / config_panel / settings.

I agree that parsing the config_panel.toml to search for default key seems a good idea.

Note that if bind = "settings" or `bind = ">FILE" is defined, we should automatically create the settings anyway...

I see to ways possible for this solution 3:

Solution 3a: parse config_panel.toml, create the settings and hydrate

So the idea here is to create the settings automatically from the default keys. It could be done via a helper or via _make_environment_for_app_script function (in app.py).

If needed, and install script could redefine the settings value if we need something dynamically defined.

During upgrade, we hydrate the new config file with settings replacing __VAR__ as usual.

I see 2 problems with this solution:

For upgrade, what if a setting is different of the value in the config file (the instance admin could have change the file by hand for example) ? I guess we could compare the value and trigger warning in this case...
It's sad to have to put VAR for all config_panel entries in the template...

Solution 3b: parse config_panel.toml, and use `ynh_write_var_in_file` to hydrate the value

We avoid to define all __VAR__ everywhere by using ynh_write_var_in_file in the ynh_add_config on the parsed default values...

ynh_write_var_in_file could replace the values in template file by the __VAR__ or directly by the known settings value...

IMPORTANT: solution 3 means more work on core and to add default keys in every config_panel.toml...

Corrupted config_panel

If install script or upgrade scripts depends of config panel, we need to be sure those one are working.

We can depends on a TOML well formed (should be ok).
We can also depends on a config script working in the version N (and not N+1) to read values (solution 1). Personally i think it's a bad idea.

Avoiding collision

Not totally related, but we probably should adapt the linter to avoid collision nightmare between config_panel keys, helpers variables, config_panel internal vars...

I quote the doc

Some short keys are forbidden cause it can interfer with config scripts (old, file_hash, types, binds, formats, changed) and you probably should avoid to use common settings name to avoid to bind your question to this settings (e.g. id, install_time, mysql_pwd, path, domain, port, db_name, current_revision, admin)

Conclusion

I think discuss it in a visio should be a good idea...

Gredin67 commented 9 months ago

While we are at it, coule we keep in mind thé use-case of having several default configurations depending on the use-case of the server app?

This coule be different .toml or different config.yaml files

See e.g. https://github.com/YunoHost-Apps/synapse_ynh/issues/403

Le 4 décembre 2023 22:02:03 GMT+01:00, Alexandre Aubin @.***> a écrit :

Yet another example today in https://github.com/YunoHost-Apps/fittrackee_ynh/pull/47 ...

-- Reply to this email directly or view it on GitHub: https://github.com/YunoHost/issues/issues/1973#issuecomment-1839470924 You are receiving this because you commented.

Message ID: @.***>

Tagadda commented 9 months ago

While we are at it, coule we keep in mind thé use-case of having several default configurations depending on the use-case of the server app?

This would be useful for Mastodon too. We would have to choose between "public instance" where LDAP is off and registration open, and "private instance" with LDAP on and registration off etc... and this would be fed to the config panel during the install and replace the call to ynh_add_config in the install script ?

YunoHost / issues