Conversation
WalkthroughAdds backup policy support across the migration system. Introduces Resource::TYPE_BACKUP_POLICY and a new Backups Policy resource class modeling backup policies. Adds Transfer backup group constants and integrates backups into resource extraction and public lists. Extends Source with getBackupsBatchSize and an abstract exportGroupBackups; concrete sources (Appwrite, CSV, Firebase, JSON, NHost, MockSource) add exportGroupBackups handlers (Appwrite imports via importBackupResource which marks resources skipped). Source reporting and supported-resources lists updated to include backups. 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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)
1426-1437: Consider surfacing the Cloud-only no-op when backups are requested.
exportGroupBackups()currently does a silent no-op. If backups are explicitly requested on CE, a non-fatal warning/log would improve operator feedback during migration runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Migration/Sources/Appwrite.php` around lines 1426 - 1437, exportGroupBackups currently silently no-ops; change it to emit a non-fatal warning when invoked so operators know backups are Cloud-only (reference: method exportGroupBackups). Use the class logger (e.g. $this->logger or the existing logging facility in this class) to log a clear warning indicating backups are Cloud-only and will not be exported for CE, include any relevant group identifier/parameters if available. Also augment reportBackups (reference: reportBackups and Resource::TYPE_BACKUP_POLICY) to log a brief informational message when backup policies are requested and you populate the report with zero, so the migration report plus logs both surface that backups are unsupported on CE.
🤖 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 1490-1495: The importBackupResource(Resource $resource): Resource
method currently sets Resource::STATUS_SUCCESS without performing any import;
change it to mark the resource as skipped or warning (e.g.,
Resource::STATUS_SKIPPED or Resource::STATUS_WARNING) and attach a clear message
like "Import not implemented" (or similar), and avoid returning success until
real import logic is implemented; update the method in the Appwrite migration
class (importBackupResource) to set the non-success status and message and
optionally log the skipped action so reports/do-not-report logic won't treat it
as migrated.
In `@src/Migration/Resources/Backups/Policy.php`:
- Around line 37-48: The fromArray() factory (Policy::fromArray) currently
assumes $array['id'] exists; add explicit validation for the required id: check
isset($array['id']) and that it's the expected type/format (e.g., non-empty
string or integer) and throw a clear InvalidArgumentException (or similar) if
missing/invalid so callers get a descriptive validation error instead of a PHP
notice; then pass the validated/casted id into new self(...) as before.
---
Nitpick comments:
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1426-1437: exportGroupBackups currently silently no-ops; change it
to emit a non-fatal warning when invoked so operators know backups are
Cloud-only (reference: method exportGroupBackups). Use the class logger (e.g.
$this->logger or the existing logging facility in this class) to log a clear
warning indicating backups are Cloud-only and will not be exported for CE,
include any relevant group identifier/parameters if available. Also augment
reportBackups (reference: reportBackups and Resource::TYPE_BACKUP_POLICY) to log
a brief informational message when backup policies are requested and you
populate the report with zero, so the migration report plus logs both surface
that backups are unsupported on CE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c038b0e9-982e-4e7e-a15b-4ab24f9c9d3f
📒 Files selected for processing (10)
src/Migration/Destinations/Appwrite.phpsrc/Migration/Resource.phpsrc/Migration/Resources/Backups/Policy.phpsrc/Migration/Source.phpsrc/Migration/Sources/Appwrite.phpsrc/Migration/Sources/CSV.phpsrc/Migration/Sources/Firebase.phpsrc/Migration/Sources/JSON.phpsrc/Migration/Sources/NHost.phpsrc/Migration/Transfer.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Migration/Destinations/Appwrite.php (1)
1490-1495:⚠️ Potential issue | 🟡 MinorInclude a skip reason when backup import is not implemented.
Line 1492 sets
STATUS_SKIPPEDwithout context. This makes migration reports harder to interpret when backups are intentionally no-op.Proposed fix
public function importBackupResource(Resource $resource): Resource { - $resource->setStatus(Resource::STATUS_SKIPPED); + $resource->setStatus( + Resource::STATUS_SKIPPED, + 'Backup policy import is not implemented for this destination' + ); return $resource; }🤖 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 1490 - 1495, In importBackupResource (class Appwrite) you should still mark the resource skipped but also record a human-readable skip reason; after setting Resource::STATUS_SKIPPED call a method on the Resource to record the reason (e.g. $resource->setSkipReason('backup import not implemented for Appwrite')) so migration reports show context — if Resource has no setSkipReason, add the reason to resource metadata (e.g. $resource->setMeta('skip_reason', 'backup import not implemented for Appwrite')) and return the resource.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1490-1495: In importBackupResource (class Appwrite) you should
still mark the resource skipped but also record a human-readable skip reason;
after setting Resource::STATUS_SKIPPED call a method on the Resource to record
the reason (e.g. $resource->setSkipReason('backup import not implemented for
Appwrite')) so migration reports show context — if Resource has no
setSkipReason, add the reason to resource metadata (e.g.
$resource->setMeta('skip_reason', 'backup import not implemented for Appwrite'))
and return the resource.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4aa7918e-6e48-4aa6-906c-b2e688117d89
📒 Files selected for processing (2)
src/Migration/Destinations/Appwrite.phptests/Migration/Unit/Adapters/MockSource.php
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge after fixing the missing null-coalescing guard on One P1 defect in src/Migration/Resources/Backups/Policy.php — the Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch 'main' into backup-migratio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2 participants
Summary
backup-policyresource type andbackupsgroup to the migration frameworkPolicyresource class with properties: id, name, services, retention, schedule, enabled, resourceId, resourceTypeexportGroupBackupsabstract method toSourcewith implementations in all source classesTest plan
Summary by CodeRabbit