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

MAGECLOUD-343: Method for exporting a DB Backup of production site #94

Merged
merged 18 commits into from
Nov 29, 2017
Merged

MAGECLOUD-343: Method for exporting a DB Backup of production site #94

merged 18 commits into from
Nov 29, 2017

Conversation

NadiyaS
Copy link
Contributor

@NadiyaS NadiyaS commented Nov 14, 2017

MAGECLOUD-343 Method for exporting a DB Backup of production site

Description

New CLI command to create DB dump on CLOUD

Zephyr Tests

  1. https://jira.corp.magento.com/browse/MAGETWO-83527

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • Pull request was approved by architect
  • Pull request was approved by QA member
  • 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)

}
fclose($lockFileHandle);
} catch (\Exception $e) {
$this->logger->info('ERROR: ' . $e->getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can use ->error method instead of ->info to avoid adding 'ERROR: ' to log message

$errors = $this->shell->execute('bash -c "set -o pipefail; ' . $command . '"');

if ($errors) {
$this->logger->info('Error has occurred during mysqldump');
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can increase log level to error to emphasize message value

Copy link
Contributor

Choose a reason for hiding this comment

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

the same on line 90

@shiftedreality shiftedreality added the Progress: review PR/Issue status label Nov 15, 2017

$this->process->execute();

// As we do not know the dump file name before running script,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use \phpmock\phpunit\PHPMock class for mocking php functions. Please look into StaticContentCleanerTest class.

@shiftedreality shiftedreality added Progress: testing in progress PR/issue status and removed Progress: review PR/Issue status labels Nov 17, 2017
@andriyShevtsov
Copy link
Contributor

@NadiyaS please add link to task MAGECLOUD-343

@andriyShevtsov
Copy link
Contributor

QA approved

@shiftedreality shiftedreality added Progress: accept PR/issue status and removed Progress: testing in progress PR/issue status labels Nov 20, 2017
*/
private function getCommand()
{
$command = "timeout 3600 mysqldump -h {$this->connectionData->getHost()} -P {$this->connectionData->getPort()}"
Copy link

Choose a reason for hiding this comment

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

Value 3600 should be at least cons

Copy link
Contributor Author

@NadiyaS NadiyaS Nov 20, 2017

Choose a reason for hiding this comment

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

Ok, I'll move this value to constant

*
* @return string
*/
public function getHost();
Copy link
Member

Choose a reason for hiding this comment

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

Add PHP strict types (:int, :string, etc to all methods you introduced)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cannot add strict types to all methods as some of them can return several types. Also, we need to review class Environment which returns values of environment variables, because now almost all methods return string type even if the variable does not exist. And these changes may affect current Interface as it uses Environment class

@shiftedreality shiftedreality added the Progress: on hold PR/issue status label Nov 21, 2017
@YPyltiai
Copy link

Will be re-worked a bit. After discussion agreed to add / remove some items.

@shiftedreality shiftedreality added Progress: on hold PR/issue status and removed Progress: on hold PR/issue status labels Nov 28, 2017
@NadiyaS NadiyaS removed the Progress: on hold PR/issue status label Nov 28, 2017
@shiftedreality shiftedreality merged commit 4bac8fe into magento:develop Nov 29, 2017
@shiftedreality shiftedreality deleted the MAGECLOUD-343 branch November 29, 2017 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Progress: accept PR/issue status Release: 2002.0.5 ECE-Tools Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants