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 --no-backup option to databse:copy and database:copy:all #125

Merged
merged 5 commits into from
Oct 17, 2020

Conversation

earthday47
Copy link
Contributor

Resolves #124

@typhonius
Copy link
Owner

Good idea - thanks for your contribution!

Couple of small comments.

  • Please add tests (I've written tests for most of the other methods so you should be able to add another item to https://github.com/typhonius/acquia_cli/blob/master/tests/Commands/DbCommandTest.php to get expected output. You can run the tests locally with composer test
  • Can we also alter the backupAndMoveDbs method to use your new moveDbs method so we're doing all db moving in one place.
  • I've got no-backup functionality in the code deploy task.
    if (!$options['no-backup']) {
    so I think you could alter your commit to use if (!$options['no-backup']) { for consistency.
  • Couple of minor phpcs fixes that have been identified by CI.
FILE: ...ome/travis/build/typhonius/acquia_cli/src/Commands/DbCommand.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 129 | ERROR | [x] Expected 1 space after closing brace; newline
     |       |     found
 163 | ERROR | [x] Expected 1 space after closing brace; newline
     |       |     found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

@typhonius
Copy link
Owner

I've also just switched CI over to GitHub Actions so if you rebase your branch against master you should get some improved code reviews/checks.

@earthday47
Copy link
Contributor Author

hi @typhonius thank you! All of your feedback sounds good to me, I'll get to work on them

@earthday47
Copy link
Contributor Author

Updated with tests... it looks like my force push cancelled the GitHub checks, but composer test is clean on my local.

@typhonius
Copy link
Owner

Thanks, I’ve seen that error on other PRs and looks like it’s an unrelated step later where we rebuild the phar tool. I’ll push an update later on today because I have an inkling about how to fix it.

Copy link
Owner

@typhonius typhonius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are able to rebase (again) against master and fix the following two minor changes then I'll merge it :)

if (!$options['no-backup']) {
$this->moveDbs($uuid, $environmentFrom, $environmentTo);
} else {
$this->moveDbs($uuid, $environmentFrom, $environmentTo, null, $backup = false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch $backup = false to just false.

if (!$options['no-backup']) {
$this->moveDbs($uuid, $environmentFrom, $environmentTo, $dbName);
} else {
$this->moveDbs($uuid, $environmentFrom, $environmentTo, $dbName, $backup = false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch $backup = false to just false.

@earthday47
Copy link
Contributor Author

All green! 😁
Thanks for your attention and quick review of this request - we already use the CLI as a crucial part of our workflow and this will only help improve it more.

@typhonius typhonius merged commit 12ca1d8 into typhonius:master Oct 17, 2020
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.

Option to disable backups when moving DBs
2 participants