Skip to content
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

Closed
wants to merge 6 commits into from

Conversation

timkelty
Copy link
Contributor

@timkelty timkelty commented Mar 13, 2024

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

  • make small tweaks easy (eg ignore a single table). Currently this can only be done via event.
  • make it easy to opt-in to postgres's archive format (possibly make this the default for v5?)
  • make it clear when things are going to be replaced or merged (eg ignore-tables)
  • provide a way to tap into command for modification
  • be non-breaking

Valid values include:

<?php
use craft\config\GeneralConfig;
use mikehaertl\shellcommand\Command as ShellCommand;

return GeneralConfig::create()
    ->restoreCommand([
        'archiveFormat' => true, // pgsql-only

        // example: add an arg
        'callback' => fn(ShellCommand $command) => $command->addArg('--verbose'),
    ])
    ->backupCommand([
        'ignoreTables' => ['foo', 'bar'],
        'archiveFormat' => true, // pgsql-only

        // example: remove an arg
        'callback' => function (ShellCommand $command) {
            $command->setArgs(str_replace('--dump-date', '', $command->getArgs()));

            return $command;
        },
    ]);

etc…

  • I ended up leveraging mikehaertl\shellcommand\Command because it was there and were already using it. Easier than stitching strings together.
  • To keep backward compat, ignoreTables can either come from the EVENT_BEFORE_CREATE_BACKUP or from the new ignoreTables option. If both are present, the event will take precedence.
  • The 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.

@brandonkelly brandonkelly changed the base branch from develop to 4.9 March 14, 2024 01:17
@brandonkelly
Copy link
Member

The options array is keyed by the db driver name

This feels like unnecessary complexity to me. No project uses both.

@timkelty
Copy link
Contributor Author

timkelty commented Mar 14, 2024

@brandonkelly yep, already nixed it. Flipping to draft while I tweak some things.

@timkelty timkelty marked this pull request as draft March 14, 2024 01:22
@timkelty timkelty self-assigned this Mar 14, 2024
@timkelty timkelty marked this pull request as ready for review March 14, 2024 18:49
private function _pgpasswordCommand(): string
{
return Platform::isWindows() ? 'set PGPASSWORD="{password}" && ' : 'PGPASSWORD="{password}" ';
}
Copy link
Contributor Author

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;
}
Copy link
Contributor Author

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;
Copy link
Contributor Author

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}"';
Copy link
Contributor Author

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);
Copy link
Contributor Author

@timkelty timkelty Mar 14, 2024

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}"';
Copy link
Contributor Author

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

@brandonkelly
Copy link
Member

@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 config/general.php via App::env().)

@timkelty
Copy link
Contributor Author

Closed in favor of #14897

@timkelty timkelty closed this Apr 29, 2024
@brandonkelly brandonkelly deleted the feature/db-command-options branch April 30, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants