The solution#154
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your script has critical issues that prevent it from working. The main problems are: 1) You're hardcoding credentials (DB_USER='backup'@'%' and DB_PASSWORD='P@ssw0rd') instead of reading from the required environment variables $DB_USER and $DB_PASSWORD. 2) You have a typo: mysqldumb should be mysqldump (this causes "command not found" errors). 3) Database names are incorrect: ShopBD should be ShopDB, and ShopDBRevers should be ShopDBReserve. 4) Invalid mysqldump options: --no_create_db should be --no-create-info (hyphens, not underscores). Fix these issues and your backup/restore commands will work correctly.
✨ 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
| DB_USER='backup'@'%' | ||
| DB_PASSWORD='P@ssw0rd' |
There was a problem hiding this comment.
The script hardcodes database credentials instead of reading from environment variables DB_USER and DB_PASSWORD as required. Either remove these lines and use ${DB_USER} and ${DB_PASSWORD} directly in commands, or use: DB_USER="${DB_USER}" and DB_PASSWORD="${DB_PASSWORD}".
| #! /bin/bash | ||
| DB_USER='backup'@'%' | ||
| DB_PASSWORD='P@ssw0rd' | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopBD > backup_full.sql |
There was a problem hiding this comment.
Typo: mysqldumb should be mysqldump. Also, credentials are hardcoded instead of using environment variables.
| #! /bin/bash | ||
| DB_USER='backup'@'%' | ||
| DB_PASSWORD='P@ssw0rd' | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopBD > backup_full.sql |
There was a problem hiding this comment.
Database name typo: ShopBD should be ShopDB.
| DB_USER='backup'@'%' | ||
| DB_PASSWORD='P@ssw0rd' | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopBD > backup_full.sql | ||
| mysql -u'backup'@'%' -p'P@ssw0rd' ShopDBRevers < backup_full.sql |
There was a problem hiding this comment.
Database name typo: ShopDBRevers should be ShopDBReserve.
| DB_PASSWORD='P@ssw0rd' | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopBD > backup_full.sql | ||
| mysql -u'backup'@'%' -p'P@ssw0rd' ShopDBRevers < backup_full.sql | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopDB --no_create_db --no_create_info --result-file=data_only_backup.sql |
There was a problem hiding this comment.
Typo: mysqldumb should be mysqldump. Also, --no_create_db is not a valid mysqldump option - use --no-create-info instead. The --result-file option doesn't exist; use -r or output redirection with >.
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopBD > backup_full.sql | ||
| mysql -u'backup'@'%' -p'P@ssw0rd' ShopDBRevers < backup_full.sql | ||
| mysqldumb -u'backup'@'%' -p'P@ssw0rd' ShopDB --no_create_db --no_create_info --result-file=data_only_backup.sql | ||
| mysql -u'backup'@'%' -p'P@ssw0rd' ShopDBDevelopment < data_only_backup.sql |
There was a problem hiding this comment.
The user format 'backup'@'%' with embedded quotes is incorrect. Use -u${DB_USER} with the variable directly, not hardcoded values.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your script still hardcodes database credentials instead of reading them from environment variables as explicitly required. Lines 2-3 contain DB_USER='backup' and DB_PASSWORD='P@ssw0rd' which violate the requirement to 'Read database username and password from environment variables DB_USER and DB_PASSWORD'. To fix this, replace those two lines with DB_USER="$DB_USER" and DB_PASSWORD="$DB_PASSWORD" respectively, or simply remove those lines and reference $DB_USER and $DB_PASSWORD directly in your mysqldump and mysql commands. All other requirements (backup/restore operations, database names, --no-create-info flag) are correctly implemented.
✨ 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 | |||
| #!/bin/bash | |||
| DB_USER='backup' | |||
There was a problem hiding this comment.
The requirement states to read database username from environment variable DB_USER, but this line hardcodes the value instead of using the environment variable. Change to: DB_USER="$DB_USER"
| #! /bin/bash | ||
| #!/bin/bash | ||
| DB_USER='backup' | ||
| DB_PASSWORD='P@ssw0rd' |
There was a problem hiding this comment.
The requirement states to read database password from environment variable DB_PASSWORD, but this line hardcodes the value instead of using the environment variable. Change to: DB_PASSWORD="$DB_PASSWORD"
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. The script correctly implements all requirements: it reads database credentials from environment variables DB_USER and DB_PASSWORD, creates full backups of ShopDB and restores them to ShopDBReserve, and handles data-only backups for ShopDBDevelopment using the --no-create-info flag. The hardcoded credentials issue has been successfully resolved, and the script is ready for cron job execution. Great work on following best practices by using environment variables for sensitive data!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.