YunoHost-Apps / wallabag2_ynh

Wallabag v2 package for YunoHost
https://www.wallabag.org/
GNU Affero General Public License v3.0
62 stars 14 forks source link

2.4.3 #125

Closed lapineige closed 2 years ago

lapineige commented 2 years ago

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)

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

Fingers crossed! Test Badge

lapineige commented 2 years ago

Too bad, we still have this bug https://github.com/YunoHost-Apps/wallabag2_ynh/pull/101#issuecomment-824594592 :

[console] Error thrown while running command "--no-interaction --env=prod doctrine:migrations:migrate". Message: "An exception occurred while executing 'ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL': Attention : SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL; check that it exists" ["exception" => Doctrine\DBAL\Exception\DriverException { ā€¦},"command" => "--no-interaction --env=prod doctrine:migrations:migrate","message" => """ An exception occurred while executing 'ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL':\n \n SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL; check that it exists """] Attention : In AbstractMySQLDriver.php line 128: Attention : An exception occurred while executing 'ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL': Attention : SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL; check that it exists Attention : In Exception.php line 18: Attention : SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL; check that it exists Attention : In PDOConnection.php line 141: Attention : SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY FK_RANDOMALPHANUMERICAL; check that it exists Attention : doctrine:migrations:migrate [--write-sql [WRITE-SQL]] [--dry-run] [--query-time] [--allow-no-migration] [--configuration [CONFIGURATION]] [--db-configuration [DB-CONFIGURATION]] [--db DB] [--em EM] [--shard SHARD] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] [] Attention : [Error] Upgrade failed.

cc @yalh76

lapineige commented 2 years ago

Things are progressing very fast on that specific issue: https://github.com/wallabag/wallabag/issues/5233

It seems that any installation that was done after that specific commit https://github.com/wallabag/wallabag/commit/2680b0bc8c9044b19b80a596f0005a1051b4ee54 in November 2017 is not affected. Any Wallabag installation older than November 2017/ version 2.3.0 is probably affected by this issue.

We will probably need to implement a specific upgrade command for these ones. The diagnosis is still work in progress, so we will need to wait for that.

To all users reading this : if you can check if you can upgrade to this 2.4.3 version, that would be really appreciated. Also can you provide the date of installation of your Wallabag install ? Tip to check this in command line : yunohost app list --full then search for wallabag2 content, then install_time: field, and using that number enter the command date -d @the_number_you_found and share the result with us. You might find that in the admin interface, I don't know :sweat_smile:.

nicofrand commented 2 years ago

Sun 23 Apr 2017 12:06:44 PM UTC for me :)

kr-cr commented 2 years ago

Sun 23 Sep 16:57:31 UTC 2018

But also it seems my error was not the same so I don't know if it helps. (https://github.com/YunoHost-Apps/wallabag2_ynh/pull/106#issuecomment-869158920)

lapineige commented 2 years ago

Indeed, that's not the same. Probably unrelated ? May I ask if you have an opinion @Kdecherf ?

lapineige commented 2 years ago

So we need to implement the migration mechanism for old installations, and test it properly.

I don't really know the proper way to:

@YunoHost-Apps/apps-group is there any helper that I should use to run those MYSQL commands ? And what might be the best way to check if the DB has the mentioned issue ? @Kdecherf do you have a proposal ?

Kdecherf commented 2 years ago

@kr-cr

Sun 23 Sep 16:57:31 UTC 2018

But also it seems my error was not the same so I don't know if it helps. (#106 (comment))

The problem reported in the comment is indeed unrelated. On a side note I was unable to reproduce it on a default MariaDB 10.2 setup thus I don't understand why you are hitting this.

Kdecherf commented 2 years ago

@lapineige

@YunoHost-Apps/apps-group is there any helper that I should use to run those MYSQL commands ? And what might be the best way to check if the DB has the mentioned issue ? @Kdecherf do you have a proposal ?

I think checking for a specific foreign key should be enough to assess if you need to run a fix. FK_2B219D70A76ED395 is probably safe for that, this is the user_id FK on the entry table.

nicofrand commented 2 years ago

There are two helpers:

I think the latter would be best.

nicofrand commented 2 years ago

We can also connect as a user: ynh_mysql_connect_as.

And the SQL provided is really specific, with named keys, so I wonder if we need to even check before running it.

lapineige commented 2 years ago

Thanks a lot !

And the SQL provided is really specific, with named keys, so I wonder if we need to even check before running it.*

But what happens if it fails ? :thinking:

yalh76 commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:rocket: Test Badge

nicofrand commented 2 years ago

Thanks a lot !

And the SQL provided is really specific, with named keys, so I wonder if we need to even check before running it.*

But what happens if it fails ? thinking

What would be different from the current situation?

lapineige commented 2 years ago

One point for you šŸ˜…

I'm just wondering if we could break somethingā€¦ or worse, break it but don't fail the upgrade process, so an unnoticed issue will stay in some people installationsā€¦

lapineige commented 2 years ago

For my part I don't have the time to (try to) implement this very soon.

So here is my proposal:

What do you think ?

nicofrand commented 2 years ago

If we are only 2 or 3 to have such an old version and issue, we could even:

  1. Merge this
  2. Open a ticket describing the migration issue and comment how to fix it (either download the SQL file or copy/paste it and then run a yunohost command if possible to integrate it in database)
  3. If there are still people with the issue, implement the migration progress
lapineige commented 2 years ago

I'm not a big fan off forcing users to connect thought CLI and runs some commands, not all of them are comfortable with CLI, and I suppose it's almost as much work to implement this as of writing a good tutorial and advertising it (in a way that makes it "quite" easily discoverable for the average user). In my opinion (but obviously, it totally depends on the amount of work we can provideā€¦) upgrade should be as easy and smooth for our users as a simple UI button to press and that's it. I would like to avoid them the burden of understanding what's the issue, trying to find that the way to fix it is documented in this github repository, and so on.

I think I will implement the migration process (if no one were able to do it first), but not very soon as I don't really have the time now and as I need some testing first (that's something new for me). So I'm more discussing what should be the strategy in the mid-time. I believe we agree that it's better to merge this first and then handle the special migrationā€¦ later ?

lapineige commented 2 years ago

Is it fine for you if I merge this PR as it is (2.4.3, but no special migration tool implemented) ?

nicofrand commented 2 years ago

Yes of course!