-
Notifications
You must be signed in to change notification settings - Fork 642
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
Add options config for backup/restore commands #14586
Conversation
This feels like unnecessary complexity to me. No project uses both. |
@brandonkelly yep, already nixed it. Flipping to draft while I tweak some things. |
fee907a
to
6812e99
Compare
private function _pgpasswordCommand(): string | ||
{ | ||
return Platform::isWindows() ? 'set PGPASSWORD="{password}" && ' : 'PGPASSWORD="{password}" '; | ||
} |
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.
Moved to \craft\db\DbShellCommand
FileHelper::writeToFile($this->tempMyCnfPath, $contents, ['append']); | ||
|
||
return $this->tempMyCnfPath; | ||
} |
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.
Moved to \craft\db\DbShellCommand
' {database}' . | ||
' >> "{file}"'; | ||
|
||
return $schemaDump . ' && ' . $dataDump; |
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.
Moved to \craft\db\mysql\BackupCommand
return 'mysql' . | ||
' --defaults-file="' . $this->_createDumpConfigFile() . '"' . | ||
' {database}' . | ||
' < "{file}"'; |
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.
Moved to \craft\db\mysql\RestoreCommand
' --no-acl' . | ||
' --file="{file}"' . | ||
' --schema={schema}' . | ||
' ' . implode(' ', $ignoredTableArgs); |
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.
Moved to \craft\db\pgsql\BackupCommand
' --port={port}' . | ||
' --username={user}' . | ||
' --no-password' . | ||
' < "{file}"'; |
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.
Moved to \craft\db\pgsql\RestoreCommand
@timkelty After reading #14648 (comment) I’m wondering if it would make more sense to just add a bunch of new top-level config settings for this. Brad is suggesting using a multi-env config in response, which we are sortof moving away from in favor of environment variables, but it doesn’t seem like it will be possible to use environment variables here as currently implemented. (Besides pulling them in manually from |
Closed in favor of #14897 |
Description
The PR allows
\craft\config\GeneralConfig::backupCommand
and\craft\config\GeneralConfig::restoreCommand
to be an options array to configure how the commands are generated.Goals
ignore-tables
)Valid values include:
etc…
mikehaertl\shellcommand\Command
because it was there and were already using it. Easier than stitching strings together.ignoreTables
can either come from theEVENT_BEFORE_CREATE_BACKUP
or from the newignoreTables
option. If both are present, the event will take precedence.callback
could easily be an event instead, but it seemed like it might be a pain to add an event listener just to add an additional arg.