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 option to keep Files in Rollback #11750

Merged
merged 2 commits into from
Nov 30, 2017

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Oct 25, 2017

Add option to keep files in rollback and improve Readability and PSR of Backup Library

Description

Ask to user when launch command: bin/magento setup:rollback if he wants keep files to future rollbacks. Also I changed variable name protected with underscore to acomplish PSR and I imported all classes to use in class.

Fixed Issues (if relevant)

  1. [2.1.1 CE] Rollback/Restore deletes database (--db) backup file in ${webroot}/var/backups. #6460: [2.1.1 CE] Rollback/Restore deletes database (--db) backup file in ${webroot}/var/backups.

Manual testing scenarios

  1. create backup e.g: bin/magento setup:backup --media
  2. rollback backup e.g: bin/magento setup:rollback -m 1508950479_filesystem_media.tgz
  3. Answer y to question: you want to keep the backups?[y/N]
  4. Check that your backup was kept in var/backups

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

foreach ($file as $statement) {
$this->getResourceModel()->runCommand($statement);
}
@unlink($source);
if ($this->keepSourceFile()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The logic seems to be reversed here and should be !$this->keepSourceFile()

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check tomorrow, maybe I was wrong copying between branches

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@osrecio
Copy link
Member Author

osrecio commented Oct 27, 2017

Hi @fooman , I've fixed these problems:

  1. The problem with lib/internal/Magento/Framework/Backup/Db.php, I changed condition to if (!$this->keepSourceFile()) {
  2. Problem with tests
  3. Problem with codeStyle

Same for #11742 and #11737

@magento-engcom-team magento-engcom-team added 2.2.x bugfix Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Component: Setup labels Nov 7, 2017
@okorshenko okorshenko added this to the November 2017 milestone Nov 7, 2017
@osrecio
Copy link
Member Author

osrecio commented Nov 9, 2017

@fooman I want to make some changes that I spoke with @dmanners during the #MM17ES and that are already applied in the PR: #11742 .

I hope that today I can apply them for this Backport.

@fooman fooman assigned dmanners and unassigned fooman and magento-engcom-team Nov 9, 2017
*
* @var boolean
*/
protected $keepSourceFile;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this private.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made Private

* @param boolean $keepSourceFile
* @return $this
*/
public function setKeepSourceFile($keepSourceFile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a new interface for these methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to SourceFileInterface

*
* @return boolean
*/
public function keepSourceFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move these to a new interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to SourceFileInterface

@@ -127,7 +127,13 @@ protected function execute(InputInterface $input, OutputInterface $output)
if (!$helper->ask($input, $output, $question) && $input->isInteractive()) {
return \Magento\Framework\Console\Cli::RETURN_FAILURE;
}
$this->doRollback($input, $output);
$questionKeep = new ConfirmationQuestion(
'<info>you want to keep the backups?[y/N]<info>',
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "Do you want to keep the backup files?" as the message here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed typo

@osrecio osrecio force-pushed the PR#KeepFilesRollback_2.3 branch from 6f5b992 to fd034dc Compare November 25, 2017 19:18
@osrecio
Copy link
Member Author

osrecio commented Nov 25, 2017

@dmanners Added changes, I hope everything are Ok ;-)

@magento-team magento-team merged commit 166cd7e into magento:2.3-develop Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Award: complex Award: special achievement Award: test coverage bugfix Component: Setup Progress: accept Release Line: 2.3 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants