Add live backup copy support for partial dev-env db sync and db export#2476
Add live backup copy support for partial dev-env db sync and db export#2476rinatkhaziev merged 28 commits intotrunkfrom
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
/changelog |
AI-Generated Changelog Entry
|
4edc7ad to
eb14388
Compare
Co-authored-by: Volodymyr Kolesnykov <volodymyr.kolesnykov@automattic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
9e9b33a to
5214e24
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Replace backupConfigFile parameter with liveBackupCopyCLIOptions structure - Add support for --table, --subsite-id, and --wpcli-command CLI options - Implement parseLiveBackupCopyCLIOptions utility with validation - Add comprehensive test coverage for new CLI options functionality - Update dev-env-sync-sql and export-sql commands to use new options structure This refactoring provides more flexible and type-safe handling of live backup copy configurations while maintaining backward compatibility.
- Add comma-separated parsing for --table and --subsite-id flags - Update help examples to demonstrate new syntax
f986c7a to
9745fea
Compare
|
I'm only putting DO NOT MERGE because the docs. I'm going to publish the pre-release based on this branch. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds live backup copy functionality to enable downloading or syncing partial database dumps from production environments. Instead of downloading full database exports, users can now specify custom configurations to sync/download only specific tables, subsites, or configs generated by custom WP-CLI commands.
Key changes:
- Added
--config-fileoption for JSON configuration files that define partial backup parameters - Implemented live backup copy integration with
generateLiveBackupCopy()method - Enhanced polling utilities with timeout support and error handling
Reviewed Changes
Copilot reviewed 8 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils.ts | Enhanced pollUntil function with timeout support and return value handling |
| src/lib/live-backup-copy.ts | New module implementing live backup copy functionality with GraphQL mutations |
| src/lib/http/download-file.ts | New utility for downloading files with progress tracking |
| src/lib/backup-storage-availability/backup-storage-availability.ts | Refactored to accept archive size as parameter instead of constructor injection |
| src/commands/export-sql.ts | Enhanced to support live backup copy options and refactored download logic |
| src/commands/dev-env-sync-sql.ts | Updated to support live backup copy options |
| tests/lib/backup-storage-availability/backup-storage-availability.ts | Updated tests to match new constructor signature |
| tests/commands/export-sql.ts | Added comprehensive tests for live backup copy functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let done = false; | ||
| while ( ! done ) { | ||
| isDone: ( v: T ) => boolean, | ||
| timeoutMs: number = 6 * 60 * 60 * 1000 // Default to 6 hours |
There was a problem hiding this comment.
[nitpick] The 6-hour default timeout seems excessive and could lead to operations hanging for too long. Consider using a more reasonable default like 30 minutes (30 * 60 * 1000) or making this configurable based on the operation type.
| timeoutMs: number = 6 * 60 * 60 * 1000 // Default to 6 hours | |
| timeoutMs: number = 30 * 60 * 1000 // Default to 30 minutes |
| export interface DBLiveCopyConfig { | ||
| tool?: SQLDumpTool; | ||
| type: BackupLiveCopyType; | ||
| tables?: Record< string, Record< string, string | boolean > >; |
There was a problem hiding this comment.
The type definition for tables is overly permissive. Consider defining a more specific interface for table options to improve type safety and API clarity, such as Record<string, TableOptions> where TableOptions has specific properties like where?: string, skipAddDropTable?: boolean, etc.
| tables?: Record< string, Record< string, string | boolean > >; | |
| export interface TableOptions { | |
| where?: string; | |
| skipAddDropTable?: boolean; | |
| // Add other specific options as needed | |
| } | |
| export interface DBLiveCopyConfig { | |
| tool?: SQLDumpTool; | |
| type: BackupLiveCopyType; | |
| tables?: Record<string, TableOptions>; |
| appId, | ||
| environmentId, | ||
| copyId, | ||
| timeoutInSeconds = 2 * 60 * 60, // 2 hours default timeout |
There was a problem hiding this comment.
[nitpick] The 2-hour default timeout for getting download URL is very long and inconsistent with the 6-hour polling timeout. Consider using consistent timeout values or making them configurable based on operation complexity.
| timeoutInSeconds = 2 * 60 * 60, // 2 hours default timeout | |
| timeoutInSeconds = DEFAULT_POLLING_TIMEOUT_SECONDS, // 6 hours default timeout (consistent with polling) |
| result.data?.generateLiveBackupCopyDownloadURL?.url | ||
| ? result.data?.generateLiveBackupCopyDownloadURL?.url | ||
| : 'Unknown error' | ||
| }` |
There was a problem hiding this comment.
The error message construction is confusing - it checks for url existence but then uses url in the error message when it should be describing the actual failure. Consider using a more descriptive error message like 'Failed to generate download URL: operation was not successful' or similar.
| }` | |
| `Failed to generate download URL: operation was not successful. ` + | |
| `Missing or invalid fields: ` + | |
| `success=${result.data?.generateLiveBackupCopyDownloadURL?.success}, ` + | |
| `url=${result.data?.generateLiveBackupCopyDownloadURL?.url}, ` + | |
| `size=${result.data?.generateLiveBackupCopyDownloadURL?.size}` |
| } | ||
|
|
||
| throw new Error( | ||
| `Failed to write file to disk: ${ error instanceof Error ? error.message : 'Unknown error' }` |
There was a problem hiding this comment.
Using console.error directly in a library function is not ideal as it bypasses any logging configuration the application might have. Consider using a proper logging mechanism or allowing the caller to handle this error appropriately.
| `Failed to write file to disk: ${ error instanceof Error ? error.message : 'Unknown error' }` | |
| let unlinkErrorMessage = ''; | |
| try { | |
| await fs.promises.unlink( destinationPath ); | |
| } catch ( unlinkErr ) { | |
| unlinkErrorMessage = `; Additionally, failed to delete partial file ${ destinationPath }: ${ unlinkErr instanceof Error ? unlinkErr.message : String(unlinkErr) }`; | |
| } | |
| throw new Error( | |
| `Failed to write file to disk: ${ error instanceof Error ? error.message : 'Unknown error' }${ unlinkErrorMessage }` |
|
|
||
| if ( this.liveBackupCopyCLIOptions?.subsiteIds ) { | ||
| type = BackupLiveCopyType.SUBSITE_IDS; | ||
| subsiteIds = this.liveBackupCopyCLIOptions?.subsiteIds.map( id => Number( id ) ); |
There was a problem hiding this comment.
Calling Number() on values that are already numbers (as defined in the interface) is unnecessary and could potentially cause issues. The subsiteIds property is already typed as number[], so this conversion is redundant.
| subsiteIds = this.liveBackupCopyCLIOptions?.subsiteIds.map( id => Number( id ) ); | |
| subsiteIds = this.liveBackupCopyCLIOptions?.subsiteIds; |
| if ( ! fs.existsSync( configFile ) ) { | ||
| throw new Error( `Configuration file not found: ${ configFile }` ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Using fs.existsSync() followed by fs.readFileSync() creates a race condition where the file could be deleted between the existence check and the read operation. Consider using a try-catch block around fs.readFileSync() instead to handle the file not existing.
|



Description
This PR adds live backup copy functionality to the
export sqlanddev-env sync sqlcommands, enabling developers to download or sync partial database dumps from production environments. Instead of downloading full database exports, users can now specify custom configurations to sync/download only specific tables, subsites, or configs generated by custom WP-CLI commands.Changes Made
--config-fileoption: Added support for JSON configuration files that define partial backup parametersgenerateLiveBackupCopy()method that creates on-demand backups based on configurationfiles
Configuration Examples
Table-specific backup with conditions
{ "type": "tables", "tool": "mysqldump", "tables": { "wp_posts": { "where": "post_date >= NOW() - INTERVAL 30 DAY OR post_modified >= NOW() - INTERVAL 30 DAY" }, "wp_users": { "where": "ID > 100" "skip-add-drop-table": true, "replace": true }, "wp_options": { "skip-add-drop-table": true, "replace": true } } }Subsite-specific backup (multisite)
{ "type": "subsite_ids", "tool": "mysqldump", "subsiteIds": [1, 2, 5] }Custom WP-CLI command
{ "type": "wpcli_command", "tool": "mysqldump", "wpcliCommand": "custom-db-copy-config --since='2025-01-01'" }Changelog Description
Added
Removed
Fixed
Changed
Pull request checklist
New release checklist
Steps to Test
Outline the steps to test and verify the PR here.
Example:
npm run build./dist/bin/vip-cookies.js nom