done#160
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The script has a critical violation of the core requirements: lines 3-4 hardcode the database credentials (export DB_USER=backup and export DB_PASSWORD=P@ssw0rd) instead of reading from pre-exported environment variables. The task explicitly requires reading credentials from DB_USER and DB_PASSWORD environment variables that are exported before running the script. Your script should reference ${DB_USER} and ${DB_PASSWORD} directly in the mysqldump/mysql commands, or add validation to fail if they are unset. Remove the hardcoded exports entirely. Additionally, note that the MySQL privilege errors shown in check runs are a test environment configuration issue (in create-user.sql), not a code problem in task.sh. Minor improvements (non-blocking): The password assignment lacks quotes: export DB_PASSWORD=P@ssw0rd should use quotes for robustness with special characters; consider adding set -e for error handling on failed database operations; the --result-file option may not be portable; consider using output redirection or the -r flag instead.
✨ 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
| #! /bin/bash | ||
| #!/bin/bash | ||
|
|
||
| export DB_USER=backup |
There was a problem hiding this comment.
Critical issue: This line hardcodes the database username instead of reading from the environment variable DB_USER. The task requires reading from environment variables that are exported before running the script. Either reference the variable: DB_USER="${DB_USER}" or use it directly in mysqldump commands, or add validation to fail if not set.
| #!/bin/bash | ||
|
|
||
| export DB_USER=backup | ||
| export DB_PASSWORD=P@ssw0rd |
There was a problem hiding this comment.
Critical issue: This line hardcodes the database password instead of reading from the environment variable DB_PASSWORD. The task requires reading from environment variables that are exported before running the script. Either reference the variable: DB_PASSWORD="${DB_PASSWORD}" or use it directly in mysqldump commands, or add validation to fail if not set.
| export DB_USER=backup | ||
| export DB_PASSWORD=P@ssw0rd | ||
|
|
||
| mysqldump -u "$DB_USER" -p"$DB_PASSWORD" --databases ShopDB --result-file=ShopDBReserve_backup.sql --no-create-db |
There was a problem hiding this comment.
The --result-file option may not be standard across all MySQL versions. The conventional approach is to use output redirection > or the -r option. Additionally, consider using --single-transaction for InnoDB tables to ensure data consistency.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed to meet the task requirements. The script still hardcodes database credentials on lines 3-4 (export DB_USER=backup and export DB_PASSWORD=P@ssw0rd), but the task explicitly requires reading these values from pre-exported environment variables. The user is expected to run export DB_USER="backup" and export DB_PASSWORD="<password>" before executing the script. Remove lines 3-4 entirely and reference ${DB_USER} and ${DB_PASSWORD} directly in the mysqldump and mysql commands. The backup logic and restore functionality appear correct otherwise.
✨ 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,10 @@ | |||
| #! /bin/bash | |||
|
|
|||
| export DB_USER=backup | |||
There was a problem hiding this comment.
CRITICAL: This line hardcodes the database username. The task requires reading from a pre-exported environment variable DB_USER, not setting it. Remove this line entirely and remove line 4 as well. The user will export DB_USER and DB_PASSWORD before running the script.
| #! /bin/bash | ||
|
|
||
| export DB_USER=backup | ||
| export DB_PASSWORD=P@ssw0rd |
There was a problem hiding this comment.
CRITICAL: This line hardcodes the database password. The task requires reading from a pre-exported environment variable DB_PASSWORD, not setting it. Remove this line. The credentials should come from export DB_USER="backup" and export DB_PASSWORD="<password>" executed by the user before running the script.
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 credentials from environment variables ${DB_USER} and ${DB_PASSWORD} instead of hardcoding them, properly creates a full backup of ShopDB and restores it to ShopDBReserve, and successfully migrates only the data (using --no-create-info) to ShopDBDevelopment. The script is executable without arguments as specified. Great work on properly escaping the variables with quotes for security—using "$DB_USER" and "$DB_PASSWORD" prevents issues with special characters in credentials. This is exactly the right approach for production scripts. Well done!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.