-
Notifications
You must be signed in to change notification settings - Fork 5
Case 27611; Support for s3 #25
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
Conversation
'version' => 'latest', | ||
]; | ||
$client = new S3Client($options); | ||
$client->registerStreamWrapper(); |
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.
What is the actual need for the stream wrapper?
} | ||
else { | ||
// Open a stream in read-only mode | ||
if ($stream = fopen($filename, 'r')) { |
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.
I can't see a case where the code would fall into this routine, also, this seems to be doing the same as in https://github.com/dennisinteractive/drupal_console_commands/blob/master/src/Command/SiteDbImportCommand.php#L197
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.
I think it would be a good idea to move the bit of logic to deal with copying the dump in execute() into download() and let it deal with the source (local, s3, etc)
} | ||
} | ||
else { | ||
throw new SiteCommandException("The DB dump could not be saved to the local destination."); |
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.
We need to make sure that this won't stop the site being installed from the scratch if the dump is not found
} | ||
|
||
// If the db dump is not local, download it locally. | ||
if (!stream_is_local($this->filename)) { |
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.
Maybe we need some extra check for the existence of the file (in case of nfs mount) and use the stream wrapper to check the file size to avoid copying it again if it the size did not change since last execution
Added support for s3.
It will check if it's an s3 dump, then copy it locally.