Skip to content

Fix tracking of auto disabled apps in Updater #16507

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 15, 2019
Merged

Fix tracking of auto disabled apps in Updater #16507

merged 2 commits into from
Aug 15, 2019

Conversation

@georgehrke
Copy link
Member

fixes #15416

@georgehrke georgehrke added the 3. to review Waiting for reviews label Jul 22, 2019
@georgehrke georgehrke requested a review from nickvergessen July 22, 2019 19:29
@georgehrke georgehrke mentioned this pull request Jul 22, 2019
Closed
@georgehrke
Copy link
Member Author

@nickvergessen I'm not too deep into \OC_App, so any feedback is welcome.

@georgehrke
Copy link
Member Author

Would like to have some review / comment whether this is the fix we are looking for.
Afterwards we should add some tests to make sure it doesn't break again.

nickvergessen
nickvergessen reviewed Jul 23, 2019
View reviewed changes
lib/private/Updater.php Outdated
throw new \UnexpectedValueException('The files of the app "' . $app . '" were not correctly replaced before running the update');
}
\OC::$server->getAppManager()->disableApp($app);
$this->autoDisabledApps[] = $app;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead we move the logic to the appmanager method with a bool flag.
So when someone adds another spot that disables an app, they are reminded about the option.

@nickvergessen nickvergessen self-assigned this Jul 23, 2019
@nickvergessen
Copy link
Member

Added a commit, I think that's the cleaner way to do it.

georgehrke
georgehrke commented Jul 23, 2019
View reviewed changes
@kyrofa
Copy link
Member

kyrofa commented Jul 24, 2019

Thanks for fixing this, @georgehrke. I assume backporting to v16 is also part of the plan, here, but wanted to mention it anyway.

@nickvergessen
Copy link
Member

Failing tests are

PHP Fatal error: Uncaught PDOException: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away in /drone/src/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:41

So time to rebase?

@nickvergessen
Copy link
Member

/backport to stable16

@nickvergessen
Copy link
Member

/backport to stable15

MorrisJobke
MorrisJobke approved these changes Jul 29, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense and all usages of disableApp that are affected are covered 👍

@MorrisJobke MorrisJobke force-pushed the bugfix/15416/update_autodisabled_apps branch from c9d713d to c3752ef Compare July 29, 2019 12:50
@MorrisJobke
Copy link
Member

So time to rebase?

Rebased and auto squashed.

@MorrisJobke MorrisJobke added this to the Nextcloud 17 milestone Jul 29, 2019
@kyrofa
Copy link
Member

kyrofa commented Aug 5, 2019

Still have a few test failures, here.

@the-sane the-sane mentioned this pull request Aug 6, 2019
Merged
@MorrisJobke MorrisJobke mentioned this pull request Aug 8, 2019
Merged
28 tasks
georgehrke and others added 2 commits August 15, 2019 11:12
@georgehrke @rullzer
Fix tracking of auto disabled apps in Updater
f6c3424 
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@nickvergessen @rullzer
Make the auto-disabled list more broad
810ee7d 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@rullzer rullzer force-pushed the bugfix/15416/update_autodisabled_apps branch from c3752ef to 810ee7d Compare August 15, 2019 09:12
@kyrofa
Copy link
Member

kyrofa commented Aug 15, 2019 •
edited
Loading

Hey folks, what's the hold up here? We were really hoping this would be in 16.0.4.

@rullzer rullzer merged commit 9c2d701 into master Aug 15, 2019
@rullzer rullzer deleted the bugfix/15416/update_autodisabled_apps branch August 15, 2019 18:09
@backportbot-nextcloud
Copy link

The backport to stable16 failed. Please do this backport manually.

@backportbot-nextcloud
Copy link

The backport to stable15 failed. Please do this backport manually.

@kyrofa kyrofa mentioned this pull request Aug 15, 2019
Merged
@kyrofa
Copy link
Member

kyrofa commented Aug 15, 2019

The backport to stable16 failed. Please do this backport manually.

Backport to stable16 is #16753.

rullzer pushed a commit that referenced this pull request Sep 4, 2019
@georgehrke @kyrofa
Fix tracking of auto disabled apps in Updater
639ff3d 
This is a backport of #16507 (fixing #15416) for stable16.

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@kyrofa kyrofa mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

Assignees

@nickvergessen nickvergessen

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 17

Development

Successfully merging this pull request may close these issues.

Full automatic app update option in setting Gui

6 participants

@georgehrke @nickvergessen @kyrofa @MorrisJobke @rullzer