Package improvement - Githubissues

Josue-T commented 4 years ago

Flowing https://github.com/tulir/mautrix-whatsapp/issues/193#issuecomment-636988917 I'll give you a short feedback of your package.

You shouldn't modify the homeserver.yaml config file. ~~This file is modified at each update of the synapse package. Instead you should put a specific file in conf.d directory.~~ I purpose to create a directory in /etc/matrix-synapse/app-service. And the idea is to put your registration file in this directory. This file will be named mautrix_whatsapp.yaml. And when the synapse package is updated all file which is in /etc/matrix-synapse/app-service will be automatically integrated in the app service configuration of synapse.
installing a php configuration for this app not needed, so just clean everything everywhere.
here you should use a the official way to manage template as in the example.
here you should use the action feature of the synapse package to do that, just don't update directly the database.
And just clean all not useful comment.
About backup/restore, I think you just add a requirement that synapse is already installed. There are not a really clean way to manage, but just doing some test before the restoration should be enough.

Gredin67 commented 4 years ago

Thanks a lot for the feedback. I started cleaning up the install script. Removed php and nginx files. I'm wondering if I should keep the comments explaining the bot configuration file. Could be useful if someone wants to add more configuration functionalities to the yunohost package.

About modifying homeserver.yaml is it OK if I modify once it at installation? Otherwise the bot would not work before the next time synapse will be updated. I guess in the remove script the registration file and the corresponding line in homeserver.yaml should also be removed, otherwise synapse will throw an error till next update (this is acceptable I guess). Then, for backup, restore, update, no modification to the homeserver.yaml would be made anymore. My idea is to keep following in install script:

if [ynh_replace_string --match_string="#app_service_config_files:"]
then
ynh_replace_string --match_string="#app_service_config_files:" --replace_string="app_service_config_files:" --target_file="$synapse_config_path/homeserver.yaml"
ynh_replace_string --match_string="#  - app_service_1.yaml" --replace_string="  - '$app_service_registration_path/$app.yaml'" --target_file="$synapse_config_path/homeserver.yaml"
else
ynh_replace_string --match_string="app_service_config_files:" --replace_string="app_service_config_files:\n  - '$app_service_registration_path/$app.yaml'" --target_file="$synapse_config_path/homeserver.yaml"
fi

Josue-T commented 4 years ago

My idea is this:

On synapse side: Make a small script which would be executed by the bridge to update the configuration. In this, the configuration will be updated and the synapse service will be reloaded. For this I could do it.

On the bridge/appservice side: Just call the synapse script and check that the result is true. The idea is that:

cp ../conf/registration.yaml /etc/matrix-$synapse_instance_name/app-service/mautrix_whatsapp.yaml
ynh_replace_string --match_string=__AS_TOKEN__ --replace_string="$as_token" --target_file=/etc/matrix-$synapse_instance_name/app-service/mautrix_whatsapp.yaml
ynh_replace_string --match_string=__HS_TOKEN__ --replace_string="$hs_token" --target_file=/etc/matrix-$synapse_instance_name/app-service/mautrix_whatsapp.yaml
if ! /opt/yunohost/matrix-$synapse_instance_name/update_synapse_for_appservice.sh
then
  ynh_die "Synapse can't restart with the appservice configuration"
fi

Gredin67 commented 4 years ago

* [here](https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/install#L265) you should use a the official way to manage template as in the example.

You mean something like this? https://github.com/YunoHost-Apps/synapse_ynh/blob/testing/scripts/install#L302L336

* [here](https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/install#L312) you should use the action feature of the synapse package to do that, just don't update directly the database.

I don't get your point here, can you give me some hint?

Josue-T commented 4 years ago

* [here](https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/install#L265) you should use a the official way to manage template as in the example.
You mean something like this? https://github.com/YunoHost-Apps/synapse_ynh/blob/testing/scripts/install#L302L336

Well, the idea is that on your package you just use the code that I gave you the last post. And everything else will be managed by the code that I'll write.

Josue-T commented 4 years ago

* [here](https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/blob/master/scripts/install#L312) you should use the action feature of the synapse package to do that, just don't update directly the database.

I don't get your point here, can you give me some hint?

Something like

yunohost app action run $synapse_instance_name set_admin_user -a username=$username

Gredin67 commented 4 years ago

I think I did most of the job. Can you check? Did you implement the update_synapse_for_appservice.sh script. See https://github.com/YunoHost-Apps/mautrix_whatsapp_ynh/pull/11

Josue-T commented 4 years ago

I think I did most of the job. Can you check? Did you implement the update_synapse_for_appservice.sh script. See #11

Sorry for the long time. Now I've implemented the script on the testing branch. You can try your app with this version of synapse.

Gredin67 commented 4 years ago

@Josue-T I tried getting the mautrix_whatsapp bot installed in the buster branch with your script. After solving some issues, I get following message: https://paste.yunohost.org/raw/orofafigeh

Full log of install script https://paste.yunohost.org/raw/ipiyojohat

Josue-T commented 4 years ago

Hello,

Seeing your log I think that you need to add the whatsappbot in the database before to create set it as admin. So you probably need to start mautrix-whatsapp to do this.

Gredin67 commented 4 years ago

https://paste.yunohost.org/raw/ecizirukuf Even putting the app action at the end of the install script gives the same error. Either I should add a wait command between mautrix-whatsapp service start and synapse app action, or there is an issue in the action.

Anyway if I do not use the action, then the install is working. Thus, we've got a new app :). Just still need to use template for the config file, clean up a bit, and run package check.

Josue-T commented 4 years ago

Maybe I could try to debug this issue on a VM. Do you have git branch that I can use to debug this ?

Gredin67 commented 4 years ago

Would be nice! Try the buster branch.

Le 30 août 2020 22:14:20 GMT+02:00, Josue-T notifications@github.com a écrit :

Maybe I could try to debug this issue on a VM. Do you have git branch that I can use to debug this ?

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

-- Envoyé de /e/ Mail.

Gredin67 commented 4 years ago

I think you need to create the folder /etc/matrix-synapse/app-service And I still have this error log regarding app action https://paste.yunohost.org/raw/tojodomipo

Gredin67 commented 4 years ago

I did required changes in https://github.com/YunoHost-Apps/synapse_ynh/pull/213

YunoHost-Apps / mautrix_whatsapp_ynh

Package improvement #14