Don't override (new) official templates with old ones

lapineige commented 3 years ago

Fix #65 #67

Problem

Templates download from official source are overridden with old version of them during upgrade

Solution

don't copy old template if an official template file already exist

PR Status

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

lapineige commented 3 years ago

This is blocking a lot of (old) users as current Shaarli version is not compatible with their (old) templates, so I think it's important to merge this ASAP… if someone with an old template could test this… :pray:

lapineige commented 3 years ago

Actually I hoped that this would already be tested here 😅 And as it's a blocking issue (users can no longer use the app after an upgrade) I would merge it as soon as possible.

ping @nicofrand @e-jim

lapineige commented 3 years ago

Just changed version number, I always forget that without this change automatic update won't be possible.

nicofrand commented 3 years ago

Hi, sorry I am really busy with personal things those last weeks (and following too), I didn't have time to test the MR, but this seems fine indeed!

ericgaspar commented 3 years ago

Is this PR ready to be merged ? 👀

lapineige commented 3 years ago

I can't test it myself, and I didn't have any feedback so I'm not sure… but I'm kind of willing to YOLOmerge anyway… But I don't have the rights to do that.

alexgarel commented 3 years ago

This problem hit me hard, for it render new version of ynh non usable.

So I advocate for this commit.

alexgarel commented 3 years ago

I'm voluntary to test if someone point me to some explanations on how to do it !

lapineige commented 3 years ago

Oh, I just realised I lost my rights to merge a PR on that repo… @YunoHost-Apps/apps-group is this linked to the fact that shaarli maintainers team was removed ?

I you're willing to test: I don't remember/know if that's possible with the web interface, sorry. But if using the command line is ok for you, here the steps:

make a backup: the upgrade script will do it for you too, but we never know :) (and it should be a small backup)
as admin or root : yunohost app upgrade shaarli -u https://github.com/YunoHost-Apps/shaarli_ynh/tree/template-override-fix-1

Then just try to connect to shaarli. If something bad happens, it will display a command to share the full upgrade log, please do :)

alexgarel commented 3 years ago

@lapineige I tried, but it did not work: the tpl directory is still the same (not the one from the product).

yunohost app upgrade shaarli -u https://github.com/YunoHost-Apps/shaarli_
ynh/tree/template-override-fix-1
Info: Now upgrading shaarli...
Info: [+...................] > Loading installation settings...
Info: [#++.................] > Ensuring downward compatibility...
Info: [###++...............] > Backing up the app before upgrading (may take a while)...
Info: [#####+..............] > Upgrading NGINX web server configuration...
Info: [######++............] > Making sure dedicated system user exists...
Info: [########++..........] > Upgrading PHP-FPM configuration...
Info: [##########+.........] > Upgrading logrotate configuration...
Info: [###########++.......] > Securing files and directories...
Info: [#############++.....] > Reconfiguring Fail2Ban...
Info: The service fail2ban has correctly executed the action reload-or-restart.
Info: [###############+....] > Reloading NGINX web server...
Info: [################++..] > Upgrade of shaarli completed
Success! shaarli upgraded
Success! Upgrade complete

It seems it does not pass in the part of the script you changed. I don't get Upgrading source files... on the console.

I see this is guarded by if [ "$upgrade_type" == "UPGRADE_APP" ]

How can I make it an UPGRADE_APP ? (sorry I don't have the time to deep dive into yunohost to understand that !).

lapineige commented 3 years ago

Hum, it seems that the condition is not met, but I don't understand why (and what's that condition anyway).

@Josue-T, sorry to bother you, but since you are the author of https://github.com/YunoHost/yunohost/pull/864 maybe you can't have some input here:

is that condition a valid check ? https://github.com/YunoHost-Apps/shaarli_ynh/blob/54e83502257e9221cbb2ddb705d42cc070b9c750/scripts/upgrade#L68
in which case is UPGRADE_APP going to happen ? In this case the version is different : https://github.com/YunoHost-Apps/shaarli_ynh/pull/66/files#diff-57aeb84da86cb7420dfedd8e49bc644fb799d5413d01927a0417bde753e8922f. But I'm unsure whereas it's the package version or the app version that is different. Maybe the app version is here 0.12.1 and the package version is ynh2 ? If that's the case, then it would not work when upgrading from 0.12.1~ynh1 to 0.12.1~ynh2. @alexgarel do you know which version you were using ? Latest version ?

PS : I am discovering a whole new thing here concerning app packaging 😂 😅

ericgaspar commented 3 years ago

Let's merge to testing and test from there...

lapineige commented 3 years ago

In fact I was making a commit in template-override-fix1 😅

lapineige commented 3 years ago

To sum up:

https://github.com/YunoHost-Apps/shaarli_ynh/blob/54e83502257e9221cbb2ddb705d42cc070b9c750/scripts/upgrade#L68 "$upgrade_type" == "UPGRADE_APP" condition seems wrong in some cases (upgrade from same app version, but different package version). This should be fine, except for users that had this issue with 0.12.1 and try to upgrade to this branch… which is 0.12.1 too, but package version ynh2. I'm not that sure about it, please correct me if needed :)
this was not applied during a force upgrade, and I suppose it's not applied during a force update that upgrade the app version. That is fixed by #68

Hence, until we fix that first point, @alexgarel you should be able to upgrade with yunohost app upgrade shaarli -u https://github.com/YunoHost-Apps/shaarli_ynh/tree/template-override-fix-1 --force (please note the --force) and get the fix.

YunoHost-Apps / shaarli_ynh