Check for missing app code before upgrade by VicDeo · Pull Request #27047 · owncloud/core

VicDeo · 2017-01-27T16:31:38Z

Description

Related Issue

#26227

Motivation and Context

See issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-01-27T16:31:40Z

@VicDeo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @ringmaster and @PVince81 to be potential reviewers.

VicDeo · 2017-01-30T14:39:17Z

@PVince81 what do you think?

codecov-io · 2017-01-30T17:32:29Z

Codecov Report

❗ No coverage uploaded for pull request head (preupgrade@0d08c82).

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1e31f0...0d08c82. Read the comment docs.

DeepDiver1975 · 2017-01-30T19:15:20Z

+	$preUpdate = new \OC\Updater\PreUpdate(\OC::$server->getAppManager());
+	$missingApps = $preUpdate->getMissingApps();
+	if (count($missingApps) !== 0){
+		$eventSource->send('notice', (string)$l->t('Code is missing for the following apps:'));


How does this look like in the web ui? Please add screenshot

Any reason why this is being converted to a string? It should be returning a string already 😕

@jvillafanez dunno, just like all the rest of $l->t() invocations in this file

DeepDiver1975 · 2017-01-30T19:15:47Z

+		foreach ($missingApps as $appId){
+			$eventSource->send('notice', $appId);
+		}
+		$eventSource->send('failure', (string)$l->t('Please use occ app:disable command if you don\'t need these apps or restore the code.'));


Kind of hard from the web ui ....

my comment remains valid I guess .... what about auto-disabling them when accessing via web? or in a second request?

@DeepDiver1975 ok, will do

DeepDiver1975 · 2017-01-30T19:16:16Z

+/**
+ * @author Victor Dubiniuk <dubiniuk@owncloud.com>
+ *
+ * @copyright Copyright (c) 2017, ownCloud GmbH.


remove the dot please

VicDeo · 2017-01-30T19:39:21Z

@DeepDiver1975

jvillafanez · 2017-02-20T12:46:15Z

+		$this
+			->setName('upgrade:checkapps')
+			->setDescription('Check if there are enabled apps with missing code.')
+		;


I'd move this up. It looks weird.

@jvillafanez sorry, what's exactly wrong here?

It's code style. Having a ; isolated there is weird.

@jvillafanez thanks for clarification, updated.

jvillafanez · 2017-02-20T12:49:42Z

+			if ($input->getOption('output') === self::OUTPUT_FORMAT_PLAIN){
+				$output->writeln('Code is missing for the following apps:');
+				$this->writeArrayInOutputFormat($input, $output, $missingApps);
+				$output->writeln('Please use occ app:disable command if you don\'t need these apps or restore the code.');


I trust this works fine. I wouldn't like that the command the users is supposed to execute to fix the situation fails. Please confirm.

deo@jah-mobile:~/public_html/oc-tmp> php occ app:list Enabled: - admin_audit: 0.7 - comments: 0.3.0 - dav: 0.2.8 - enterprise_key: true - federatedfilesharing: 0.3.0 - federation: 0.1.0 - files: 1.5.1 - files_sharing: 0.10.0 - files_versions: 1.3.0 - gallery: 15.0.0 - provisioning_api: 0.5.0 - systemtags: 0.3.0 - updatenotification: 0.2.1 Disabled: - activity - encryption - files_antivirus - files_drop - files_external - files_texteditor - files_trashbin - files_videoviewer - richdocuments - templateeditor - testing deo@jah-mobile:~/public_html/oc-tmp> mv apps/admin_audit/ ../admin_audit deo@jah-mobile:~/public_html/oc-tmp> php occ upgrade:checkapps Code is missing for the following apps: - admin_audit Please use occ app:disable command if you don't need these apps or restore the code. deo@jah-mobile:~/public_html/oc-tmp> php occ app:disable admin_audit admin_audit disabled deo@jah-mobile:~/public_html/oc-tmp> php occ upgrade:checkapps deo@jah-mobile:~/public_html/oc-tmp>

jvillafanez · 2017-02-20T12:50:23Z

+				$this->writeArrayInOutputFormat($input, $output, [ 'missing' => $missingApps ]);
+			}
+
+


Unneeded empty lines.

jvillafanez · 2017-02-20T12:52:04Z

+	$preUpdate = new \OC\Updater\PreUpdate(\OC::$server->getAppManager());
+	$missingApps = $preUpdate->getMissingApps();
+	if (count($missingApps) !== 0){
+		$eventSource->send('notice', (string)$l->t('Code is missing for the following apps:'));


Any reason why this is being converted to a string? It should be returning a string already 😕

jvillafanez · 2017-02-20T13:00:24Z

+	}
+
+	/**
+	 * @return array


Taking into account this is a public method, the return value should be clearer. Is it returning a list of strings? a list of numbers? a list of app objects? an array with a specific format such as [<appid> => true]?

jvillafanez · 2017-02-22T08:09:44Z

+
+use OCP\App\IAppManager;
+
+class PreUpdate {


I don't have a clear idea what is the purpose of this class. Why is this class needed? Why we can't move the getMissingApps method inside the AppManager, or in any other class to manage apps?

@jvillafanez This check is done before upgrade. And there will be more checks in future.

Maybe an interface IPreUpgradeCheck with a doCheck method (not sure about what parameters should be included) might be a better choice.
For a specific check you can implement the interface and do whatever you want to do inside the doCheck method.

The only potential problem would be the implementation discovery. For now, core could create the implementations he's willing to go through during the upgrade process.

@DeepDiver1975 does it make sense?

This check is done before upgrade. And there will be more checks in future.

please add this to the classÄ phpdoc

Maybe an interface IPreUpgradeCheck with a doCheck method (not sure about what parameters should be included) might be a better choice.

since we do not expect any public api usage it is okay to it this way

DeepDiver1975 · 2017-03-21T21:24:48Z

+		$missingApps = [];
+		foreach ($installedApps as $appId){
+			$info = $this->appManager->getAppInfo($appId);
+			if (!isset($info['id'])){


I guess you should better test for null

@DeepDiver1975 May be both. What if there is no key id?

VicDeo · 2017-03-24T18:20:16Z

@DeepDiver1975 updated, rebased, squashed.
CLI still offer to take any action on their own

Web UI autodisables the app:

VicDeo · 2017-03-29T17:16:41Z

@DeepDiver1975 so, is it good enough now?

PVince81 · 2017-04-07T11:56:54Z

Tested, works.

However in the web UI I think the update should abort if apps have missing code.

The reason is that if the app that is missing has some important routines like user backends, disabling that app is likely to break things because some migration code iterates over ALL users. But in the case of a missing user backend, some users would not be migrated at all.

The CLI behavior is fine.

PVince81 · 2017-04-07T11:57:41Z

and then comes the question: what about people who don't have CLI access, how would they disable these apps manually ? Hmmm... maybe the updater needs to show an error and then provide a button "click here to disble the apps" which reloads the page with an override URL parameter.

thoughts ?

VicDeo · 2017-04-07T14:43:31Z

@PVince81 as suggested by @DeepDiver1975 it is autodisabled in web UI. (See the last commit)

PVince81 · 2017-04-07T14:46:05Z

The reason I asked for this feature was specifically to avoid migrations that forgot users due to disabled app. If we auto disable apps then we lose this and people will still result in broken envs.

I could imagine this happening in case there are bugs in the "bundled app to market migration" code where for example user_ldap used to be on-disk (repo package) but now in the new version it's not there but must be downloaded from the market. But then if download failed and the upgrade continues (we likely don't want to continue here!), then user_ldap app is missing and the migration code of other apps that need all users will not find the LDAP users.

@DeepDiver1975

PVince81 · 2017-04-07T14:46:28Z

of course user_ldap disallowes running web UI update, but some other user backends might

PVince81 · 2017-04-20T19:30:25Z

This becomes even more important when unbundling apps to the marketplace. The risk of an app's code missing is much higher.

PVince81 · 2017-04-20T19:39:20Z

This becomes even more important when unbundling apps to the marketplace. The risk of an app's code missing is much higher.

PVince81 · 2017-04-21T09:22:25Z

reuse this PR for Adjust update process to work with market market#39
make sure the web UI also aborts (for now we provide no option to disable apps from the UI, something for 10.0.1 possibly using the "secret" "disable app short-circuit API even when OC is broken" endpoint)

PVince81 · 2017-04-24T11:24:21Z

Obsoleted by #27711

lock · 2019-08-03T12:02:35Z

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.

VicDeo added the 2 - Developing label Jan 27, 2017

VicDeo force-pushed the preupgrade branch 2 times, most recently from 11e0dfc to 4d96628 Compare January 30, 2017 14:38

VicDeo changed the title ~~[WIP] Check for missing app code before upgrade~~ Check for missing app code before upgrade Jan 30, 2017

VicDeo added this to the 10.0 milestone Jan 30, 2017

DeepDiver1975 requested changes Jan 30, 2017

View reviewed changes

VicDeo added 3 - To Review and removed 2 - Developing labels Feb 20, 2017

jvillafanez reviewed Feb 20, 2017

View reviewed changes

VicDeo force-pushed the preupgrade branch 2 times, most recently from 37921c6 to b03332e Compare February 21, 2017 18:14

jvillafanez reviewed Feb 22, 2017

View reviewed changes

VicDeo force-pushed the preupgrade branch from b03332e to b049478 Compare March 21, 2017 14:18

DeepDiver1975 reviewed Mar 21, 2017

View reviewed changes

VicDeo force-pushed the preupgrade branch 4 times, most recently from 4144c2d to 5f985e1 Compare March 24, 2017 18:16

VicDeo force-pushed the preupgrade branch from 5f985e1 to f1efd09 Compare March 27, 2017 17:38

VicDeo added 3 commits April 20, 2017 18:24

Check for missing app code before upgrade #26227

933d7d0

Add an explanation. Propagate check to the web updater

905898c

Try to autodisable missing app when running from web

7a58b2a

VicDeo force-pushed the preupgrade branch from f1efd09 to 7a58b2a Compare April 20, 2017 15:49

This was referenced Apr 20, 2017

Add repair step to load missing apps before upgrade #27711

Merged

Adjust update process to work with market owncloud/market#39

Closed

PVince81 closed this Apr 24, 2017

PVince81 deleted the preupgrade branch April 24, 2017 11:24

lock Bot locked as resolved and limited conversation to collaborators Aug 3, 2019

		$this->writeArrayInOutputFormat($input, $output, [ 'missing' => $missingApps ]);
		}

Uh oh!

Conversation

VicDeo commented Jan 27, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

mention-bot commented Jan 27, 2017

Uh oh!

VicDeo commented Jan 30, 2017

Uh oh!

codecov-io commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VicDeo commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvillafanez Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VicDeo commented Mar 24, 2017

Uh oh!

VicDeo commented Mar 29, 2017

Uh oh!

PVince81 commented Apr 7, 2017

Uh oh!

PVince81 commented Apr 7, 2017

Uh oh!

VicDeo commented Apr 7, 2017

codecov-io commented Jan 30, 2017 •

edited

Loading

jvillafanez Feb 22, 2017 •

edited

Loading