'Solution'#163
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your script has two critical issues that must be addressed before approval. First, the restore to ShopDBReserve uses CREATE DATABASE IF NOT EXISTS without dropping the existing database first — this breaks disaster recovery since old data would remain merged with the restored backup. You need to add DROP DATABASE IF EXISTS ShopDBReserve; before creating the database. Second, there's no error handling (set -e or explicit || exit checks) which is essential for a nightly cron job to detect and report failures. Additionally, replace --result-file=backup.sql with the standard -r backup.sql or simple redirection (> backup.sql) for better MySQL version compatibility.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| @@ -1 +1,7 @@ | |||
| #! /bin/bash | |||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --result-file=backup-no-create-db.sql | |||
There was a problem hiding this comment.
The --result-file= flag should be replaced with the standard -r flag or just output redirection >. The --result-file= format may not work consistently across all MySQL versions.
| @@ -1 +1,7 @@ | |||
| #! /bin/bash | |||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --result-file=backup-no-create-db.sql | |||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info --result-file=backup-no-create-db-info.sql | |||
There was a problem hiding this comment.
Same issue as line 2 - --result-file= should be replaced with -r flag for better compatibility.
| @@ -1 +1,7 @@ | |||
| #! /bin/bash | |||
There was a problem hiding this comment.
Consider adding set -e at the beginning of the script to exit on any command failure. This is important for a cron job that runs nightly - you want to know if the backup or restore operation fails.
| mysql -u $DB_USER -p$DB_PASSWORD -e "CREATE DATABASE IF NOT EXISTS ShopDBReserve;" | ||
| mysql -u $DB_USER -p$DB_PASSWORD -e "CREATE DATABASE IF NOT EXISTS ShopDBDevelopment;" | ||
| mysql -u $DB_USER -p$DB_PASSWORD ShopDBReserve < backup-no-create-db.sql | ||
| mysql -u $DB_USER -p$DB_PASSWORD ShopDBDevelopment < backup-no-create-db-info.sql |
There was a problem hiding this comment.
Add cleanup commands at the end to remove temporary backup files after successful restoration to avoid leaving unnecessary files on the server.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The script still has critical issues that block proper functionality. On line 5, the DROP DATABASE IF EXISTS ShopDBReserve command remains commented out - this breaks disaster recovery because restoring a backup without dropping the existing database first will cause old data to merge with restored data instead of cleanly replacing it. Additionally, lines 6-7 show the database creation commands are still commented out, meaning the script relies on external database setup rather than being self-sufficient for automated cron job execution. To fix these issues:
- Uncomment line 5:
DROP DATABASE IF EXISTS ShopDBReserve;- This must happen before restoring the backup to ensure a clean recovery - Uncomment lines 6-7: The
CREATE DATABASE IF NOT EXISTScommands for ShopDBReserve and ShopDBDevelopment - an autonomous cron job should not depend on external database creation
The set -e addition was a good improvement, but these database management commands are essential for the script to work correctly as specified in the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| set -e | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db > backup-no-create-db.sql | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info > backup-no-create-db-info.sql | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "DROP DATABASE IF EXISTS ShopDBReserve;" |
There was a problem hiding this comment.
This line is still commented out. The DROP DATABASE IF EXISTS ShopDBReserve is critical for disaster recovery - without dropping the existing database first, old data will merge with restored data instead of being replaced. Uncomment this line.
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db > backup-no-create-db.sql | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info > backup-no-create-db-info.sql | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "DROP DATABASE IF EXISTS ShopDBReserve;" | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "CREATE DATABASE IF NOT EXISTS ShopDBReserve;" |
There was a problem hiding this comment.
This CREATE DATABASE IF NOT EXISTS is still commented out. For automated cron job execution, the script should not rely on external setup. Either uncomment this or document that the database must be pre-created.
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info > backup-no-create-db-info.sql | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "DROP DATABASE IF EXISTS ShopDBReserve;" | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "CREATE DATABASE IF NOT EXISTS ShopDBReserve;" | ||
| #mysql -u $DB_USER -p$DB_PASSWORD -e "CREATE DATABASE IF NOT EXISTS ShopDBDevelopment;" |
There was a problem hiding this comment.
This CREATE DATABASE IF NOT EXISTS is still commented out. Same issue as line 6 - for reliable automation, the database should be created within the script if needed.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational Override: This is an approval to help you progress while learning.
Your script shows solid understanding of mysqldump with proper flags like --single-transaction, --routines, --triggers, and --no-create-info for schema exclusion. Great work on reading credentials from environment variables instead of hardcoding them.
However, there's one critical gap: before restoring to ShopDBReserve, you need to add DROP DATABASE IF EXISTS ShopDBReserve; CREATE DATABASE ShopDBReserve; to ensure a clean slate for disaster recovery. Similarly, before restoring data to ShopDBDevelopment, drop existing tables or recreate the database to prevent 'table already exists' errors. Without these steps, repeated script runs will merge old data with restored data, breaking automated cron execution.
Tip: Use
mysql -e "DROP DATABASE IF EXISTS db_name; CREATE DATABASE db_name;"to prepare databases before piping in the restore commands.
Keep up the good work on the backup logic—this is a strong foundation!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| set -e | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db > backup-no-create-db.sql | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info > backup-no-create-db-info.sql | ||
| mysql -u $DB_USER -p$DB_PASSWORD ShopDBReserve < backup-no-create-db.sql |
There was a problem hiding this comment.
Before restoring to ShopDBReserve, you must drop the existing database and recreate it. Without this, old data will conflict with restored data, breaking disaster recovery.
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db > backup-no-create-db.sql | ||
| mysqldump --user=$DB_USER --password=$DB_PASSWORD ShopDB --no-create-db --no-create-info > backup-no-create-db-info.sql | ||
| mysql -u $DB_USER -p$DB_PASSWORD ShopDBReserve < backup-no-create-db.sql | ||
| mysql -u $DB_USER -p$DB_PASSWORD ShopDBDevelopment < backup-no-create-db-info.sql |
There was a problem hiding this comment.
Before restoring data to ShopDBDevelopment, drop existing tables (or use DROP DATABASE + CREATE DATABASE). This prevents 'table already exists' errors and ensures clean data restoration.
No description provided.