Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing #101

Merged
merged 16 commits into from
Jun 6, 2021
Merged

Testing #101

merged 16 commits into from
Jun 6, 2021

Conversation

lapineige
Copy link
Member

@lapineige lapineige commented Apr 9, 2021

Problem

  • New official version available

Solution

  • Update to 2.4.2

PR Status

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

ericgaspar and others added 11 commits October 22, 2020 15:16
[autopatch] Autopatch to migrate to new permission system
[autopatch] Missing ynh_abort_if_errors in change_url scripts
* Tighten permissions

* Check for existence of cache file before chmoding it
Quash the last package_check warning
CI gives us this warning:
> ! Using official helper ynh_permission_update implies requiring at least version 3.7.0, but manifest only requires 3.5
* Upgrade to upstream version 2.4.2
@lapineige
Copy link
Member Author

!testme

@yunohost-bot
Copy link
Contributor

🌻
Test Badge

@lapineige
Copy link
Member Author

lapineige commented Apr 9, 2021

Upgrade from master fails in my case:

ERROR     [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_SomeRandomNumber':
WARNING: SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY `FK_SomeRandomNumber`; 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_SomeRandomNumber':\n  \n  SQLSTATE[42000]: Syntax error or access violation: 1091 Can't DROP FOREIGN KEY `FK_SomeRandomNumber`; check that it exists  """]

@lapineige
Copy link
Member Author

@YunoHost-Apps/apps-group you probably know that better than me : I believe the CI test upgrade from latest version, but which precise version ? Latest master ?

CI says Upgrade (f75d58cb): OK but f75d58c is not the latest commit in master.

@yalh76
Copy link
Member

yalh76 commented Apr 9, 2021

@YunoHost-Apps/apps-group you probably know that better than me : I believe the CI test upgrade from latest version, but which precise version ? Latest master ?

CI says Upgrade (f75d58cb): OK but f75d58c is not the latest commit in master.

At least from the last master :)
2.3.8~ynh3 => fc84732

@lapineige
Copy link
Member Author

So to what commit that "f75d58cb" is referring to ?

Does that works also with a very recently merged commit ?

@yalh76
Copy link
Member

yalh76 commented Apr 9, 2021

in fact, it's fc847322efbede1654d4c201ffd5855e3cb53774 current master

@lapineige
Copy link
Member Author

lapineige commented Apr 10, 2021

But if I understand well the summary, it's using this very old commit from 2017 f75d58c, isn't it ?

Note: I'm trying to figure out that to know if I discovered a particular issue in my wallabag instance, or a more general bug during the upgrade.

Copy link
Member

@alexAubin alexAubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Super quick review, LGTM but not super familiar with the app)

@lapineige
Copy link
Member Author

I tested again, I have this strange bug again…

Can someone else try to upgrade and see if it works correctly ?

@nicofrand
Copy link

@lapineige I just tried a fresh install (I did not add any article though) and then an upgrade from testing and it looked OK to me: https://paste.yunohost.org/raw/loposuqubo.

@lapineige
Copy link
Member Author

Then that's an issue with my instance… great 😅

@nicofrand
Copy link

I did not test on my production instance though, I don't want to lose anything. Does the upgrade fail gracefully on your instance (ie. you don't lose anything and 2.3.8 is still working after the upgrade failure)?

@lapineige
Copy link
Member Author

lapineige commented Apr 21, 2021

Yes it does.
(I did backup my articles just in case)

If you can try it, maybe you can import a few/all articles from your production instance, and try to upgrade ?


I wonder how I could solve my bug… I don't understand it 😅

@nicofrand
Copy link

OK, I got some errors too on production: https://paste.yunohost.org/raw/vugedeceko

@nicofrand
Copy link

Also see wallabag/wallabag#4826

@lapineige
Copy link
Member Author

We are not lucky, that we waited for 2.4.1 to include those fixes, and that we still have the issue 😅

    ! Using official helper ynh_legacy_permissions_delete_all implies requiring at least version 4.1, but manifest only requires 3.7.0 
    ! Using official helper ynh_legacy_permissions_exists implies requiring at least version 4.1, but manifest only requires 3.7.0
Copy link
Member

@yalh76 yalh76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review, LGTM

@yalh76 yalh76 mentioned this pull request Apr 24, 2021
4 tasks
@yalh76
Copy link
Member

yalh76 commented Apr 24, 2021

!testme

@yunohost-bot
Copy link
Contributor

🚀
Test Badge

@lapineige
Copy link
Member Author

I think we need to wait for a feedback from wallabag/wallabag#5233

The issue I encountered doesn't seem to be specific to my instance, and the CI can't test that.
Until we find a fix, I'm against merging this to master, the risk of breaking people instances is too high in my opinion.

@yalh76 yalh76 marked this pull request as draft April 25, 2021 10:31
@yalh76
Copy link
Member

yalh76 commented Apr 25, 2021

OK then, I let you move forward on this :)

@yalh76 yalh76 marked this pull request as ready for review June 6, 2021 17:01
@yalh76 yalh76 merged commit 0dcbec9 into master Jun 6, 2021
@lapineige
Copy link
Member Author

Wait, did we merged this while you issue mentioned above is not fixed ? I hope that won't break anything…

@alexAubin
Copy link
Member

Ugh indeed, didn't read that carefully ... At least the backup process before upgrade should still work

If you or anybody can confirm the issue, we can revert the merge I guess..

@lapineige
Copy link
Member Author

lapineige commented Jun 6, 2021

Both @nicofrand and I did encounter that issue.
I would prefer to wait until we have some feedback on the related issue in wallabag official repository before merging to master.

The backup is here indeed, but I prefer that people whiling to test come here to test that PR/testing branch than the risk to break a lot of instances and rely on the backup system & stuff like that. 2 people encountering the error is a small sample, I'm not sure it's a frequent issue, but this app is used by many people and I'd prefer to stay a bit conservative, just in case. It's not that difficult to install this branch if someone really need the new version.

@alexAubin do you know if a revert can have any (bad) impact on current servers that did upgrade already ?

@alexAubin
Copy link
Member

Alrighty, I guess reverting should be okay since it was only merged hours ago ... I created a revert-of-the-revert here that we can merge once the upstream issue is resolved : #106

@lapineige
Copy link
Member Author

At first I was a bit lost, but ok now I understand, #106 is like merging this PR again when we think it is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants