-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: backup all db #133
base: master
Are you sure you want to change the base?
feat: backup all db #133
Conversation
This breaks the I'm also concerned about all other datastore plugins, and the fact that we don't have native, single-datastore multi-db support. I'd want to resolve that before implementing this (or concurrently I guess). |
I did some basic testing on the import command. Here's my testing process:
Result: I understand that you are trying to ensure DX consistency across the rest of the datastores plugins, however for now, this will unblock my workflow and I'll continue with this Let me know if there's anything else I would potentially break |
|
||
[[ -n $SSH_TTY ]] && stty -opost | ||
docker exec "$SERVICE_NAME" bash -c "printf '[client]\ndefault-character-set=utf8mb4\npassword=$PASSWORD\n' > /root/credentials.cnf" |
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.
Any reason we dropped the character set 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.
Good catch, I probably left it out by accident
docker exec -i "$SERVICE_NAME" mysql --user=root --password="$ROOTPASSWORD" "$DATABASE_NAME" | ||
|
||
docker exec "$SERVICE_NAME" bash -c "printf '[client]\nuser=root\npassword=$ROOTPASSWORD\n' > /root/credentials.cnf" | ||
docker exec -i "$SERVICE_NAME" mysql --defaults-extra-file=/root/credentials.cnf "$DATABASE_NAME" | ||
docker exec "$SERVICE_NAME" rm /root/credentials.cnf |
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.
Why this particular change?
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 change is introduced to surpress warnings from mysql
Warning: Using a password on the command line interface can be insecure.
Apologies for the late review, but I left some comments asking for clarification. Thanks for testing otherwise! |
closes #109
Instead of only backing up the primary database (which must be the same name as the mysql instance), we are now backing up all databases, including the ones manually created by hand. We are however excluding the internal dbs used by mysql itself.
With this PR, I can now create additional databases on a single mysql container, saving us compute resources and $.