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

Install nextcloud via migrations instead of the db_structure.xml #5772

Merged
merged 16 commits into from
Jul 25, 2017

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 18, 2017

Installing Nextcloud now uses migrations only instead of the old db_structure.xml
The old file was not taken into consideration on updates anymore anyway, which caused a problem for @fancycode and @Ivansss with sqlite, after the accounts table was dropped it was not recreated on update.

This is now fixed, because all the structure is in a migration file.

Fix #5646
Fix #4914

MorrisJobke
MorrisJobke previously approved these changes Jul 18, 2017
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.

Tested and works (and code looks good as well

@nickvergessen
Copy link
Member Author

@MorrisJobke I can replace all placeholders with {{...}} if you think that is better. I used the <..> because that is what OC used.

@@ -58,7 +58,6 @@
'COPYING-README',
'core',
'cron.php',
'db_structure.xml',
Copy link
Member Author

Choose a reason for hiding this comment

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

@LukasReschke mentioning you as requested for potential build script adjustments

Copy link
Member

Choose a reason for hiding this comment

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

I guess for this case no adjustment is needed, because it was a shipped file. We only should test the updater to check if the file list needs to be updated somehow.

@nickvergessen nickvergessen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jul 19, 2017
@nickvergessen
Copy link
Member Author

db_structure.xml is still used by db convert and fix charset commands, will fix.

@nickvergessen nickvergessen dismissed MorrisJobke’s stale review July 19, 2017 11:33

Dismissing review because there were many changes afterwards

@nextcloud nextcloud deleted a comment from codecov bot Jul 19, 2017
@MorrisJobke
Copy link
Member

BTW this is another step towards #2270

@nickvergessen
Copy link
Member Author

Migrations? yes. This here? no.

@codecov
Copy link

codecov bot commented Jul 19, 2017

Codecov Report

Merging #5772 into master will decrease coverage by 3.83%.
The diff coverage is 0.41%.

@@             Coverage Diff              @@
##             master    #5772      +/-   ##
============================================
- Coverage     57.06%   53.23%   -3.84%     
- Complexity    21017    22751    +1734     
============================================
  Files          1245     1403     +158     
  Lines         71040    87484   +16444     
  Branches          0     1327    +1327     
============================================
+ Hits          40537    46569    +6032     
- Misses        30503    40915   +10412
Impacted Files Coverage Δ Complexity Δ
lib/private/Setup/PostgreSQL.php 0% <ø> (ø) 14 <0> (-1) ⬇️
lib/private/Setup/Sqlite.php 0% <ø> (ø) 4 <0> (ø) ⬇️
lib/private/DB/SchemaWrapper.php 0% <ø> (ø) 14 <0> (ø) ⬇️
lib/private/Setup/AbstractDatabase.php 0% <0%> (ø) 18 <1> (ø) ⬇️
core/Command/Db/ConvertType.php 0% <0%> (ø) 55 <24> (+3) ⬆️
...nd/Db/Migrations/GenerateFromSchemaFileCommand.php 0% <0%> (ø) 17 <17> (?)
core/Migrations/Version13000Date20170718121200.php 0% <0%> (ø) 32 <32> (?)
core/register_command.php 0% <0%> (ø) 0 <0> (ø) ⬇️
core/Command/Db/Migrations/GenerateCommand.php 0% <0%> (ø) 13 <9> (+1) ⬆️
lib/private/Setup/OCI.php 0% <0%> (ø) 35 <0> (-3) ⬇️
... and 349 more

@nickvergessen nickvergessen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 25, 2017
@nickvergessen
Copy link
Member Author

Should be ready for merge now

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

AWESOME!

More tests would be nice. But lets get this in ASAP so we can catch error during development. But works like a charm here...

@LukasReschke LukasReschke merged commit 68c4fc2 into master Jul 25, 2017
@LukasReschke LukasReschke deleted the migrations-install branch July 25, 2017 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants