Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds parent-resource status validation in the Appwrite migration destination: before importing a Function or Site deployment the code checks the resolved parent Function/Site status and, if not Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Use the resources array to determine whether to export only the active deployment or all deployments. When deployment/site-deployment resource type is included, export all deployments instead of only the active one.
PHP booleans become '1'/'' in multipart/form-data but the API expects 'true'/'false' strings. The non-chunked function deployment path was already correct; this fixes the chunked function deployment and both site deployment paths.
…stination When a function or site already exists in the destination, the API returns 409 and the parent resource is marked as error. Previously, deployments for that parent would still be imported, creating duplicates. Now importDeployment and importSiteDeployment check the parent status and throw an error if it failed.
premtsd-code
force-pushed
the
fix-export-inactive-deployments
branch
from
589dd29 to
928e7c8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1521-1522: $exportOnlyActive is being set when
Resource::TYPE_DEPLOYMENT is not requested but currently code falls back to
paginated listing and exports all deployments for resources (functions/sites)
that lack an active deployment; change the callers and exportDeployments logic
so that when $exportOnlyActive is true you only export deployments explicitly
referenced by an active deployment ID and never fall back to the full paginated
export: detect resources (functions/sites) with no activeDeploymentId and skip
exporting their deployments (do not call the paginated listing), and ensure the
same change is applied to the second occurrence around the lines mentioned (the
other $exportOnlyActive usage).
…unnecessary env var checks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1476-1484: The current checks only skip child deployment imports
when the parent function status equals Resource::STATUS_ERROR, which misses
other non-success states (e.g., STATUS_SKIPPED); update the guard(s) to skip
whenever the parent status is not Resource::STATUS_SUCCESS. Specifically, change
the condition that reads $function = $deployment->getFunction(); if
($function->getStatus() === Resource::STATUS_ERROR) ... to check if
($function->getStatus() !== Resource::STATUS_SUCCESS) and call
$deployment->setStatus(Resource::STATUS_SKIPPED, ...) accordingly; apply the
same change to the other occurrence around the block referenced (the check at
the 1691–1695 area) so any non-success parent prevents child import.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Migration/Destinations/Appwrite.php (1)
1452-1459:⚠️ Potential issue | 🟠 MajorAdd parent-status guards for function/site variable imports too.
Deployments now correctly skip when parent import failed, but function/site variables still import unconditionally. On re-migrate (parent 409), this can still write child vars instead of returning the expected parent-failure skip.
💡 Proposed fix
case Resource::TYPE_ENVIRONMENT_VARIABLE: /** `@var` EnvVar $resource */ + $function = $resource->getFunc(); + if ($function->getStatus() !== Resource::STATUS_SUCCESS) { + $resource->setStatus(Resource::STATUS_SKIPPED, 'Parent function "' . $function->getId() . '" failed to import'); + return $resource; + } $this->functions->createVariable( - $resource->getFunc()->getId(), + $function->getId(), $resource->getKey(), $resource->getValue() ); break;case Resource::TYPE_SITE_VARIABLE: /** `@var` SiteEnvVar $resource */ + $site = $resource->getSite(); + if ($site->getStatus() !== Resource::STATUS_SUCCESS) { + $resource->setStatus(Resource::STATUS_SKIPPED, 'Parent site "' . $site->getId() . '" failed to import'); + return $resource; + } $this->sites->createVariable( - $resource->getSite()->getId(), + $site->getId(), $resource->getKey(), $resource->getValue() ); break;Also applies to: 1663-1669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Destinations/Appwrite.php` around lines 1452 - 1459, The EnvVar creation block (case Resource::TYPE_ENVIRONMENT_VARIABLE) currently calls $this->functions->createVariable unconditionally; change it to first check the parent import result/status and only call $this->functions->createVariable when the parent import succeeded (i.e. not failed/skipped/returned 409); if the parent failed or was skipped, skip creating the variable and mark the child as skipped. Apply the same guard to the analogous site-variable creation block (the one around lines handling site variables) so both function and site variables respect parent-status before calling createVariable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1452-1459: The EnvVar creation block (case
Resource::TYPE_ENVIRONMENT_VARIABLE) currently calls
$this->functions->createVariable unconditionally; change it to first check the
parent import result/status and only call $this->functions->createVariable when
the parent import succeeded (i.e. not failed/skipped/returned 409); if the
parent failed or was skipped, skip creating the variable and mark the child as
skipped. Apply the same guard to the analogous site-variable creation block (the
one around lines handling site variables) so both function and site variables
respect parent-status before calling createVariable.
When exportOnlyActive is true, functions/sites without an active deployment ID should be skipped entirely rather than falling back to the full paginated listing.
2 participants
Summary
activateparam to string'true'/'false'for multipart form-data (PHP booleans serialize as1/"", nottrue/false)