-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Feat split migrations #8144
base: refactor-usage-sn
Are you sure you want to change the base?
Feat split migrations #8144
Conversation
@@ -34,12 +34,6 @@ public function __construct() | |||
'default' => '', | |||
'example' => 'pending', | |||
]) | |||
->addRule('stage', [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the difference between stage and status? why is this removed?
This needs to be released as a patch version so we should not have breaking changes. If it has to be removed, then we need a response filter for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a response filter, please re-review
- _APP_MIGRATIONS_FIREBASE_CLIENT_ID | ||
- _APP_MIGRATIONS_FIREBASE_CLIENT_SECRET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're never rolling out firebase OAuth so I just removed it while I was there
@christyjacob4 I rebased this to |
@@ -4138,6 +4117,106 @@ | |||
] | |||
], | |||
], | |||
'groupMigrations' => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'groupMigrations' => [ | |
'migrationsGroup' => [ |
if (!empty(array_intersect( | ||
$groupResources, | ||
$resources | ||
))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move this to a variable and reuse it on L53
$migration = $dbForProject->createDocument('migrations', new Document([ | ||
'$id' => ID::unique(), | ||
'status' => 'pending', | ||
'stage' => 'init', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets keep the stage
attribute since its required. We can attempt to remove it during the 1.6 migrations for example
@@ -163,7 +157,7 @@ protected function updateMigrationDocument(Document $migration, Document $projec | |||
roles: $target['roles'], | |||
); | |||
|
|||
return $this->dbForProject->updateDocument('migrations', $migration->getId(), $migration); | |||
return $this->dbForProject->updateDocument($migration->getCollection(), $migration->getId(), $migration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change required?
$log->addBreadcrumb(new Breadcrumb("debug", "migration", "Migration hit stage 'processing'", \microtime(true))); | ||
$this->updateMigrationDocument($migrationDocument, $projectDocument); | ||
|
||
$log->addTag('type', $migrationDocument->getAttribute('source')); | ||
$this->updateMigrationDocument($migration, $projectDocument); | ||
|
||
$log->addTag('type', $migration->getAttribute('source')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets group the logger related statements together and the migrations related statements together
switch ($group->getAttribute('group')) { | ||
case Transfer::GROUP_AUTH: | ||
$resources = array_intersect(Transfer::GROUP_AUTH_RESOURCES, $resources); | ||
break; | ||
case Transfer::GROUP_STORAGE: | ||
$resources = array_intersect(Transfer::GROUP_STORAGE_RESOURCES, $resources); | ||
break; | ||
case Transfer::GROUP_DATABASES: | ||
$resources = array_intersect(Transfer::GROUP_DATABASES_RESOURCES, $resources); | ||
break; | ||
case Transfer::GROUP_FUNCTIONS: | ||
$resources = array_intersect(Transfer::GROUP_FUNCTIONS_RESOURCES, $resources); | ||
break; | ||
default: | ||
throw new Exception('Migration worker was initialized with unknown group'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You already did an array_intersect in the API level. This doesn't seem to be required
|
||
$status = $document->getAttribute('status', 'processing'); | ||
|
||
if ($status == 'processing' || $status == 'pending') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($status == 'processing' || $status == 'pending') { | |
if ($status === 'processing' || $status === 'pending') { |
if ($status == 'failed') { | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ($status == 'failed') { | |
break; | |
} | |
if ($status === 'failed') { | |
$result = 'failed'; | |
break; | |
} |
What does this PR do?
This PR splits migrations out so it can be handled by multiple workers
Test Plan
Tested locally for Appwrite, still need to test firebase, and other adapters. Will tick off as I go
This is a DB Breaking change, a migration will be needed.
Checklist