-
Notifications
You must be signed in to change notification settings - Fork 2
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
Move Acquia DB backups to cloud v2. #218
Conversation
protected function getLatestBackup($client, $environment_uuid) { | ||
$backup = new DatabaseBackups($client); | ||
$backups = $backup->getAll($environment_uuid, $this->database); | ||
$filepath = $this->dir . '/' . $this->env . '_' . $this->database . '_' . 'sql.gz'; |
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.
@acidaniel would it make sense to also set the file to load from backups based on this value?
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.
@jesconstantine The load from backups task will load any *sql.gz
file that exists in the artifact/backups
directory regardless the name.
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.
@acidaniel I think that's my concern. I think manual backups have a different file patterns but could get loaded instead of the one that we download from prod. To me this is the same issue as what we were discussing in slack here - but am I think about it correctly?
I added this to our build.yml
to ensure the correct backup was loaded: https://github.com/palantirnet/wisdhs-public/pull/566/files#diff-0fad5e53ffd9ff1b0af504ff6c4d0961af03f34af69d9e00de527f2f29c9cb2bR50-R56
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.
@jesconstantine I would love to see your commit, but I got 404 message :'( (you know why. Rub salt in the wound).
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.
@jesconstantine I just changed here the extension, I had a typo, please let me know if there something else to do here.
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.
@jesconstantine I think I found a way to improve performance by 96% of loading database with only using Linux commands. Here is a bit of background:
- Using
drupal-load-db
was using agunzip -c
command that when I run it it took around 10 minutes to finish the import - Using
<exec executable="ddev"...
took around 56 seconds to load, but this is not an option since we need to ensure we are outside the container. So this option is discarded. - Figured out and reading that by using native Linux commands like
gzip -dc
in combination with a directmysql
command takes only 46 seconds
You can see how I am implementing it really straightforward. So following the general formula about performance improvement in percentage which is:
( new - old ) / old * 100%
We have:
(42 - 600)/600*100= -93
What do you think about this?
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'm so sorry about posting that link, I wasn't thinking. 😦
Here's the snippet:
build:
# Configuration for the database loading utility.
load_db:
# Pattern to match gzipped dump files
export_pattern: "./artifacts/*.sql.gz"
file: "./artifacts/dhsinternet_d9_db_backup.sql.gz"
…ed if acquia is selected as hosting during the project setup.
@jesconstantine This is ready to be reviewed now. Since the package was removed now I'm getting issues in CircleCI, do you think we need to skip this file to be linted? |
targets/install.xml
Outdated
<!-- Copy .env.example file to store Acquia Cloud Credentials --> | ||
<copy file="${build.thebuild.dir}/defaults/install/.env.example" todir="${application.startdir}" overwrite="true"/> |
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 know the .env.example
currently only has Acquia data in it, but I could see it being useful for non-acquia hosted projects too, what do you think?
Should we copy the file somewhere more globally and add the platform specific data to it here instead?
@acidaniel you asked:
Can we either:
What do you think? |
…lected as hosting provider and remove copy of the .env.example to move it some more globally.
…to improve performance by 93%
…, we don't need this.
defaults.yml
Outdated
@@ -110,7 +113,7 @@ drupal: | |||
# Database connection for Drupal. | |||
database: | |||
# This value is required, but a default is set in the-build.xml. | |||
# database: drupal | |||
database: db |
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.
Do we need to change the default in the-build.xml instead of overriding it by default here?
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.
Instead of setting a default value here for the database name (which could break multisite behavior), I updated the default for the first database that gets copied with defaults/install/the-build/build.yml
. See 90b7d7d
Now, when a multisite is added, and that default is not present, it defaults to the site directory name (as it did before this PR).
I confirmed the testing steps work on an existing project using this branch. I think we just need to address the issues mentioned above, and this will be good to go. |
@@ -361,10 +196,15 @@ protected function validate() { | |||
if (empty($this->database)) { | |||
$this->database = $this->site; | |||
} | |||
// Check .env file exists. | |||
if (!file_exists(".env")) { | |||
throw new \BuildException(".env file is needed in the root of project with credentials see .env.example."); |
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.
This is checking for the .env
file in the root, but it actually needs to exist in the .ddev
directory for the environment variables to be created. I'm going to remove this, because the API already checks for the presence of these variables and fails accordingly:
example > acquia-get-backup:
[getacquiabackup] Couldn't find ACQUIA_CLOUD_API_KEY env variable.
[getacquiabackup] Couldn't find ACQUIA_CLOUD_API_SECRET env variable.
BUILD FAILED
This was successfully tested on another client project and ready to merge. |
https://palantir.atlassian.net/browse/DEV-32
Test Intructions:
Add this to an existing project and run
composer install
Ensure in the
build.yml
from the existing project you have the following variables:Ensure there is a
.env
with acquia clould credentials specified in the 1PasswordRun
ddev restart
Run
ddev phing acquia-get-backup
Ensure backup was downloaded inside the backups directory specified in
backups
Run
ddev phing drupal-load-db
and ensure database dump downloaded is imported correctly.