Replay migrations on the created schema before copying data #30643

VicDeo · 2018-02-27T21:34:47Z

answer 'n' to the 'continue' question was ignored
added a new line after progressbar completion. Because this sucks:

oc_account_terms
    0 [>---------------------------]oc_accounts
 1/1 [============================] 100%oc_addressbookchanges

now it looks like this:

oc_account_terms
    0 [>---------------------------]
oc_accounts
 1/1 [============================] 100%
oc_addressbookchanges

codecov · 2018-02-27T21:44:16Z

Codecov Report

Merging #30643 into master will increase coverage by 0.09%.
The diff coverage is 37.09%.

@@             Coverage Diff              @@
##             master   #30643      +/-   ##
============================================
+ Coverage     65.53%   65.63%   +0.09%     
- Complexity    18648    18663      +15     
============================================
  Files          1218     1218              
  Lines         70549    70588      +39     
  Branches       1288     1288              
============================================
+ Hits          46236    46331      +95     
+ Misses        23936    23880      -56     
  Partials        377      377

Flag	Coverage Δ	Complexity Δ
#javascript	`53.69% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67% <37.09%> (+0.1%)`	`18663 <37> (+15)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
core/register_command.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/files_external/appinfo/app.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Command/Db/ConvertType.php	`38.27% <36.69%> (+38.27%)`	`56 <37> (+15)`	⬆️
apps/systemtags/appinfo/app.php	`14.28% <75%> (-0.35%)`	`0 <0> (ø)`
lib/private/DB/Connection.php	`67.64% <0%> (+1.47%)`	`49% <0%> (ø)`	⬇️

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 5f787f9...8552b8a. Read the comment docs.

VicDeo · 2018-02-28T07:44:56Z

@PVince81 it fixes the conversion but it needs to be polished and covered with tests

PVince81 · 2018-02-28T08:32:12Z

apps/files_external/appinfo/app.php

@@ -35,7 +35,7 @@
 $appContainer = \OC_Mount_Config::$app->getContainer();

 $config = \OC::$server->getConfig();
-if ($config->getAppValue('core', 'enable_external_storage', 'no') === 'yes') {
+if (class_exists('OCA\Files\App') && $config->getAppValue('core', 'enable_external_storage', 'no') === 'yes') {


unrelated ? can you submit this as a separate tech debt PR ?

@PVince81
Related due to a cross-app dependency which is not a subject of this PR. I need to load app to read its migrations.
If files app is not loaded (it doesn't use migrations) - result of loading files_external (using migrations) is 💥

VicDeo · 2018-03-07T19:58:32Z

core/Command/Db/ConvertType.php

-		$apps = $input->getOption('all-apps') ? \OC_App::getAllApps() : \OC_App::getEnabledApps();
+		$this->replayMigrations($fromDB, $toDB, 'core');
+
+		$apps = $input->getOption('all-apps') ? $this->appManager->getAllApps() : $this->appManager->getInstalledApps();


getAllApps is not what we need here

PVince81 · 2018-03-19T10:08:32Z

core/Command/Db/ConvertType.php

+		$sourceMigrationService = new MigrationService($app, $fromDB);
+		$currentMigration = $sourceMigrationService->getMigration('current');
+		if ($currentMigration !== '0') {
+			$targetMigrationService = new MigrationService($app, $toDB);


are you not filtering migrations by "SQL" only ?

Consider that some migrations are operating on data, for example making some conversion.

When converting the DB we don't need to re-process the data as it was already processed.

This only works if all migrations work correctly with already converted data and have no side effect.
We should likely make it a requirement that every migration must be "replayable" in a way that it doesn't break if the prerequisite was already been resolved before (ex: already migrated data)

@DeepDiver1975

@PVince81 target migrations table will not have these migrations if I skip them. As the target DB is a bare structure with no data we need to replay everything on it.
Otherwise these migrations can be reapplied after the data is copied = applied twice.

Good point. In any case it seems sensible to make sure that non-schema migrations work correctly when replayed

PVince81 · 2018-04-03T10:17:47Z

what's left to do ?

VicDeo · 2019-02-12T14:02:41Z

@PVince81

would this be safe to merge and doesn't affect other code paths?

No, it doesn't affect other code paths.

PVince81 · 2019-03-07T14:35:05Z

I'm merging this as it would bring the command in a more usable state, potentially with remaining bugs to uncover. We should advertise this as beta / experimental for now until more testing is done.

@pmaier1 FYI

PVince81 · 2019-03-07T14:35:26Z

@VicDeo can you add a note on the CLI help / output that this is currently experimental ?

TobeStokes · 2019-03-22T20:01:38Z

I'm trying to follow this and figure out what the status is for the database conversion tool... I have OC 10.0.10.4 installed but unfortunately using SQLite and want to convert to MariaDB. I found out the hard way about the table limit in SQLite last December. Managed to recover and stabilize everything, but really want to get away from SQLite.

Is the DB conversion tool usable in 10.0.10 yet?

VicDeo · 2019-04-05T09:15:35Z

@PVince81

can you add a note on the CLI help / output that this is currently experimental ?

done

@TobeStokes

Is the DB conversion tool usable in 10.0.10 yet?

No, it isn't

core/Command/Db/ConvertType.php

VicDeo · 2019-04-05T12:04:26Z

@jvillafanez thanks for your efforts but I have no plans to invest more time into this 1 year old PR until it will be planned for the sprint.
Reason: any conceptual change will take too much time for manual testing.

jvillafanez · 2019-04-05T12:05:40Z

I think you're using the class as a data holder (for the DB parameters)... not sure if we have time to improve that.

PVince81 · 2019-04-05T12:18:21Z

we should at least finish and ship as beta. for that we need a review.
if it turns out that adjusting it takes more time, we can schedule this again.

pmaier1 · 2019-05-06T08:51:49Z

we should at least finish and ship as beta.

Yes, if you feel confident now, let's include it in the next minor and mention that it's beta / waiting for feedback.

VicDeo · 2019-05-13T21:06:26Z

@jvillafanez please rereview

jvillafanez · 2019-05-14T07:51:42Z

How much time do we have or do we want to invest here? There are several changes we should consider to improve the code quality, but those will take some time.
I haven't checked in depth the possibilities of unittests in the command, but I'm pretty sure it isn't fully covered with just 3 tests, plus the tests aren't isolated: the MDB2SchemaManager and MigrationService classes aren't mocked.

michaelstingl · 2019-05-23T11:33:56Z

@micbar @pmaier1 Change milestone to ~~10.3~~ development?

pmaier1 · 2019-05-23T11:48:05Z

How much time do we have or do we want to invest here?

It should work and not cause issues, of course. Anything else is out-of-scope here right now. If that's satisfied here, let's ship this with the next minor release and let people try and get feedback.

@micbar can you take this into your scope for the next release, please? THX.

jvillafanez

I assume the code works properly.
There are several code changes we should do, but we can't in the allocated time

VicDeo · 2019-05-30T14:52:17Z

brifly retested and rebased once again

VicDeo · 2019-05-31T08:37:25Z

stable10: #35390

VicDeo added the 2 - Developing label Feb 27, 2018

VicDeo added this to the development milestone Feb 27, 2018

VicDeo force-pushed the fix-27075 branch from df2fe1a to f4779c6 Compare February 27, 2018 21:44

VicDeo changed the title ~~Replay migrations after the schema is created and before copying data~~ Replay migrations on the created schema before copying data Feb 28, 2018

VicDeo changed the title ~~Replay migrations on the created schema before copying data~~ [WIP] Replay migrations on the created schema before copying data Feb 28, 2018

PVince81 reviewed Feb 28, 2018

View reviewed changes

VicDeo mentioned this pull request Feb 28, 2018

occ db:convert-type doesn't work with migration #27075

Open

VicDeo self-assigned this Feb 28, 2018

VicDeo force-pushed the fix-27075 branch 6 times, most recently from bb54729 to fb5489b Compare March 5, 2018 21:54

VicDeo changed the title ~~[WIP] Replay migrations on the created schema before copying data~~ Replay migrations on the created schema before copying data Mar 5, 2018

VicDeo added 3 - To Review and removed 2 - Developing labels Mar 6, 2018

VicDeo force-pushed the fix-27075 branch 2 times, most recently from 5b03d29 to 8ea2f87 Compare March 6, 2018 21:33

VicDeo commented Mar 7, 2018

View reviewed changes

ownclouders added the status/STALE label Mar 15, 2018

VicDeo removed the status/STALE label Mar 16, 2018

VicDeo force-pushed the fix-27075 branch from dbad846 to 076c62f Compare March 16, 2018 18:23

PVince81 reviewed Mar 19, 2018

View reviewed changes

ownclouders added the status/STALE label Mar 27, 2018

VicDeo force-pushed the fix-27075 branch from 076c62f to f707fb9 Compare March 28, 2018 13:51

ownclouders removed the status/STALE label Mar 28, 2018

VicDeo force-pushed the fix-27075 branch from a201234 to 916122c Compare March 1, 2019 15:12

VicDeo force-pushed the fix-27075 branch from 916122c to 6a0f317 Compare April 5, 2019 09:14

PVince81 requested review from jvillafanez and DeepDiver1975 April 5, 2019 09:23

VicDeo force-pushed the fix-27075 branch from 6a0f317 to 99ce384 Compare April 5, 2019 09:24

jvillafanez reviewed Apr 5, 2019

View reviewed changes

core/Command/Db/ConvertType.php Outdated Show resolved Hide resolved

core/Command/Db/ConvertType.php Show resolved Hide resolved

core/Command/Db/ConvertType.php Outdated Show resolved Hide resolved

VicDeo force-pushed the fix-27075 branch from 99ce384 to 83c8d78 Compare May 13, 2019 21:03

jvillafanez approved these changes May 23, 2019

View reviewed changes

VicDeo added 3 commits May 30, 2019 17:37

Replay migrations on schema creation

abf1fbc

Isolate cross-app dependency

c9cc480

Some changes according to the review

8552b8a

VicDeo force-pushed the fix-27075 branch from 83c8d78 to 8552b8a Compare May 30, 2019 14:51

VicDeo merged commit 731b269 into master May 31, 2019

delete-merged-branch bot deleted the fix-27075 branch May 31, 2019 08:31

VicDeo mentioned this pull request May 31, 2019

[Stable10] Replay migrations on the created schema before copying data #35390

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replay migrations on the created schema before copying data #30643

Replay migrations on the created schema before copying data #30643

VicDeo commented Feb 27, 2018 •

edited

Loading

codecov bot commented Feb 27, 2018 •

edited

Loading

VicDeo commented Feb 28, 2018

PVince81 Feb 28, 2018

VicDeo Feb 28, 2018 •

edited

Loading

VicDeo Mar 7, 2018

PVince81 Mar 19, 2018

PVince81 Mar 19, 2018

VicDeo Mar 19, 2018 •

edited

Loading

PVince81 Mar 19, 2018

PVince81 commented Apr 3, 2018

VicDeo commented Feb 12, 2019

PVince81 commented Mar 7, 2019

PVince81 commented Mar 7, 2019

TobeStokes commented Mar 22, 2019

VicDeo commented Apr 5, 2019 •

edited

Loading

VicDeo commented Apr 5, 2019 •

edited

Loading

jvillafanez commented Apr 5, 2019

PVince81 commented Apr 5, 2019

pmaier1 commented May 6, 2019

VicDeo commented May 13, 2019

jvillafanez commented May 14, 2019

michaelstingl commented May 23, 2019 •

edited

Loading

pmaier1 commented May 23, 2019

jvillafanez left a comment

VicDeo commented May 30, 2019

VicDeo commented May 31, 2019

Replay migrations on the created schema before copying data #30643

Replay migrations on the created schema before copying data #30643

Conversation

VicDeo commented Feb 27, 2018 • edited Loading

codecov bot commented Feb 27, 2018 • edited Loading

Codecov Report

VicDeo commented Feb 28, 2018

PVince81 Feb 28, 2018

Choose a reason for hiding this comment

VicDeo Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

VicDeo Mar 7, 2018

Choose a reason for hiding this comment

PVince81 Mar 19, 2018

Choose a reason for hiding this comment

PVince81 Mar 19, 2018

Choose a reason for hiding this comment

VicDeo Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

PVince81 Mar 19, 2018

Choose a reason for hiding this comment

PVince81 commented Apr 3, 2018

VicDeo commented Feb 12, 2019

PVince81 commented Mar 7, 2019

PVince81 commented Mar 7, 2019

TobeStokes commented Mar 22, 2019

VicDeo commented Apr 5, 2019 • edited Loading

VicDeo commented Apr 5, 2019 • edited Loading

jvillafanez commented Apr 5, 2019

PVince81 commented Apr 5, 2019

pmaier1 commented May 6, 2019

VicDeo commented May 13, 2019

jvillafanez commented May 14, 2019

michaelstingl commented May 23, 2019 • edited Loading

pmaier1 commented May 23, 2019

jvillafanez left a comment

Choose a reason for hiding this comment

VicDeo commented May 30, 2019

VicDeo commented May 31, 2019

VicDeo commented Feb 27, 2018 •

edited

Loading

codecov bot commented Feb 27, 2018 •

edited

Loading

VicDeo Feb 28, 2018 •

edited

Loading

VicDeo Mar 19, 2018 •

edited

Loading

VicDeo commented Apr 5, 2019 •

edited

Loading

VicDeo commented Apr 5, 2019 •

edited

Loading

michaelstingl commented May 23, 2019 •

edited

Loading