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.

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

Revert "Dispatch upgradeAppStoreApps event" #27729

Merged
merged 3 commits into from
Oct 13, 2017
Merged

Revert "Dispatch upgradeAppStoreApps event" #27729

merged 3 commits into from
Oct 13, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Apr 24, 2017 •
edited by VicDeo
Loading

Reverts #27648 and some cleanup for upgrade process.

As suggested in #27711 (comment)

@VicDeo

@PVince81 PVince81 added this to the 10.0 milestone Apr 24, 2017
@VicDeo VicDeo mentioned this pull request Apr 24, 2017
Merged
9 tasks
@PVince81
Copy link
Contributor Author

Hmm, might as well cleanup the remaining unneeded code.

PVince81
PVince81 commented Apr 24, 2017
View reviewed changes
lib/private/Updater.php Outdated
$this->emit('\OC\Updater', 'incompatibleAppDisabled', [$app]);
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@VicDeo should we still keep this ? I'm not sure if a full revert is a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

@PVince81 yes, looks like this part had a logic flaw before

@PVince81
Copy link
Contributor Author

If we revert this it seems some of the workflow will change, so might need a full retest of the update code.

@PVince81 PVince81 modified the milestones: 10.0.1, 10.0 Apr 26, 2017
@PVince81
Copy link
Contributor Author

@VicDeo can you finish this for 10.0.1 ? Seems we'll need to keep some logic.

@PVince81
Copy link
Contributor Author

PVince81 commented May 4, 2017

@VicDeo please finish this if applicable, else close

@VicDeo
Copy link
Member

VicDeo commented May 4, 2017 •
edited
Loading

@PVince81 @DeepDiver1975 do we need auto disable as a last line of defence?
If not, I would just kill disabling/reenabling here at once

@PVince81 PVince81 modified the milestones: 10.0.2, 10.0.1 May 19, 2017
@PVince81 PVince81 modified the milestones: 10.0.2, 10.0.3 May 26, 2017
@PVince81
Copy link
Contributor Author

PVince81 commented Jul 4, 2017

UnresolvableWord: audisable

@VicDeo
Copy link
Member

VicDeo commented Jul 4, 2017

@PVince81 oh. it should sound like auto-disable

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 4, 2017

@VicDeo auto-disable on update ? I think we removed that... currently the only auto-disable that's left is if the app throws an exception in app.php, and that's during app load, not install

@VicDeo
Copy link
Member

VicDeo commented Jul 4, 2017

@PVince81 not exactly.
We removed disabling of all non-shipped apps.
While incompatible apps are still disabled here.

Another story that reaching this point is blocked by repair step.

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 4, 2017

ah yes... incompatible apps... yes, let's disable these automatically

@PVince81
Copy link
Contributor Author

PVince81 commented Aug 9, 2017

moving to "planned"

@PVince81 PVince81 modified the milestones: planned, development Aug 9, 2017
@VicDeo VicDeo force-pushed the revert-27648-update-from-market branch from c851797 to 7ecab71 Compare September 21, 2017 13:40
@VicDeo
Copy link
Member

VicDeo commented Sep 22, 2017 •
edited
Loading

Rebased.
Lines 345-356 look pretty useless to me after a rebase

@VicDeo
Copy link
Member

VicDeo commented Sep 22, 2017

@PVince81 I'm feeling like killing more code here.
Incompatibility check has been moved into before upgrade repair step. So at least checkAppsRequirements is just a useless crap. There is no need in such duplication

@PVince81
Copy link
Contributor Author

@VicDeo sounds good

@VicDeo VicDeo force-pushed the revert-27648-update-from-market branch 2 times, most recently from f76154d to 9014586 Compare October 12, 2017 13:48
@VicDeo
Copy link
Member

VicDeo commented Oct 12, 2017

Rebased. Squashed.

@VicDeo VicDeo force-pushed the revert-27648-update-from-market branch from 9014586 to f46cb38 Compare October 12, 2017 20:04
@PVince81
Copy link
Contributor Author

👍 apps are still properly fetched from the market

@PVince81 PVince81 merged commit 37fd974 into master Oct 13, 2017
@PVince81 PVince81 deleted the revert-27648-update-from-market branch October 13, 2017 12:42
@PVince81
Copy link
Contributor Author

@VicDeo please backport to stable10

@VicDeo VicDeo mentioned this pull request Oct 13, 2017
Merged
6 tasks
@VicDeo
Copy link
Member

VicDeo commented Oct 13, 2017

Stable10: #29249

@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@VicDeo VicDeo VicDeo left review comments

Assignees

@VicDeo VicDeo

Labels
3 - To Review
Projects
None yet
Milestone
10.0.5
Development

Successfully merging this pull request may close these issues.
2 participants
@PVince81 @VicDeo