Skip to content

Solution_1#166

Open
Bilik143 wants to merge 3 commits into
mate-academy:mainfrom
Bilik143:main
Open

Solution_1#166
Bilik143 wants to merge 3 commits into
mate-academy:mainfrom
Bilik143:main

Conversation

@Bilik143

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because, while your task.sh script itself correctly implements all the required backup and restore functionality, the current setup causes the tests to fail due to insufficient database privileges for the backup user. The CI logs show mysqldump: Couldn't execute 'show events': Access denied for user 'backup'@'%' to database 'ShopDB' (1044), which means the user defined in create-user.sql is missing the EVENT privilege needed when mysqldump inspects events. This is a critical blocker for task completion because the backup process cannot run successfully under the intended user in the test environment.

The specific fix needed is to update your create-user.sql (or equivalent user-creation script) to grant the EVENT privilege on ShopDB to the backup user, for example:

  • Either add a separate statement like: GRANT EVENT ON ShopDB.* TO 'backup'@'%';
  • Or extend the existing grant to include EVENT, such as: GRANT SELECT, LOCK TABLES, EXECUTE, SHOW VIEW, EVENT ON ShopDB.* TO 'backup'@'%';
    After adjusting the grants, ensure FLUSH PRIVILEGES; is present so the change takes effect and re-run the tests.

On the positive side, your bash script is robust (set -euo pipefail, trap cleanup), reads credentials from environment variables, performs a full backup to ShopDBReserve, and does a data-only restore to ShopDBDevelopment exactly as required. Once the database permissions are fixed, your solution should pass cleanly. Nice work on the scripting part; focus now on correcting the user privileges so the automated checks can succeed.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because a couple of core task requirements are still not met and will likely cause functional issues. The biggest blocker is that the required SQL user/privilege setup (including EVENT privilege and a FLUSH PRIVILEGES; after updating GRANTs) is still missing, which is explicitly part of the task and may cause mysqldump to fail for the backup user [CHECKLIST ITEM #18, #19, #20, #25, #30, #31]. Within task.sh, the main backup/restore logic is correct and uses DB_USER/DB_PASSWORD properly, but for a nightly automated script you should add set -euo pipefail near the top and handle failures so that a partial or failed backup does not silently continue. You should also address the temporary backup files (e.g. backup-*.sql) by either cleaning them up at the end of the script or writing them into a dated backup directory so they don’t accumulate indefinitely. Finally, reconsider the use of --no-create-db in the “full” backup: either remove it for a truly full dump of ShopDB, or clearly rely on your create-database.sql for database creation and keep the behavior consistent. Once the SQL privilege script is updated and the script is hardened for cron usage, your solution will align well with the task 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread task.sh
@@ -1 +1,7 @@
#! /bin/bash
#!/usr/bin/env bash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a cron‑run backup script you should add safety options like set -euo pipefail near the top so that the script stops on errors instead of silently continuing with a partial or failed backup/restore.

Comment thread task.sh Outdated
#! /bin/bash
#!/usr/bin/env bash

mysqldump -u "$DB_USER" -p"$DB_PASSWORD" ShopDB --no-create-db > backup-no-create-db.sql

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dump uses --no-create-db, which means it doesn’t include the CREATE DATABASE statement. Given the requirements talk about a full backup of ShopDB, consider whether you really want to omit DB creation here or instead rely only on create-database.sql for DB creation (and make that intention clear).

Comment thread task.sh Outdated
mysql -u "$DB_USER" -p"$DB_PASSWORD" ShopDBReserve < backup-no-create-db.sql

mysqldump -u "$DB_USER" -p"$DB_PASSWORD" ShopDB --no-create-info > backup-no-create-info.sql
mysql -u "$DB_USER" -p"$DB_PASSWORD" ShopDBDevelopment < backup-no-create-info.sql No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this script is intended for nightly automated use, consider cleaning up these temporary backup-*.sql files when you’re done or using a dedicated backup directory with timestamps so backups don’t accumulate indefinitely.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution; it now fully meets the task requirements and is well-suited for a nightly cron job. Your task.sh script correctly uses set -euo pipefail, environment variables (DB_USER, DB_PASSWORD, DB_HOST), and mktemp with a trap to ensure robust, non-interactive execution and proper cleanup. The backup flow is implemented cleanly: a full mysqldump of ShopDB is restored into ShopDBReserve, and a data-only dump using --no-create-info is restored into ShopDBDevelopment, so you avoid copying schema into the development database as required. In create-user.sql, the backup user has the appropriate minimal global privileges plus EVENT on ShopDB.*, ALL on ShopDBReserve.*, and targeted privileges on ShopDBDevelopment.*, with FLUSH PRIVILEGES; ensuring everything takes effect immediately. As a minor improvement, you could double-check that comments and naming in the SQL script clearly describe the extra EVENT privilege so future readers understand why it’s there, but functionally your setup is solid. Overall, you’ve produced a clear, reliable solution that demonstrates good scripting practices and careful handling of database permissions—well done.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread create-user.sql
@@ -1,10 +1,13 @@
CREATE USER 'backup'@'%' IDENTIFIED BY 'P@ssw0rd';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Including set -euo pipefail at the top is appropriate for a nightly cron job and ensures the script fails fast instead of continuing after an error, which aligns well with the backup/recovery intent.

Comment thread create-user.sql

-- configure minimum requored permissions for performing a db backup using mysqldump:
-- configure minimum required permissions for performing a db backup using mysqldump:
GRANT SELECT, LOCK TABLES, SHOW VIEW, PROCESS ON *.* TO 'backup'@'%';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using parameter expansion to require DB_USER is set matches the requirement to read credentials from environment variables and gives a clear error if it’s missing.

Comment thread create-user.sql
-- configure minimum requored permissions for performing a db backup using mysqldump:
-- configure minimum required permissions for performing a db backup using mysqldump:
GRANT SELECT, LOCK TABLES, SHOW VIEW, PROCESS ON *.* TO 'backup'@'%';
GRANT EVENT ON ShopDB.* TO 'backup'@'%';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding GRANT EVENT for ShopDB covers event-related permissions that mysqldump may need when dumping a database with events, improving reliability for the backup user.

Comment thread create-user.sql
-- configure required permissions to restore only data
GRANT INSERT, LOCK TABLES, ALTER ON ShopDBDevelopment.* TO 'backup'@'%';

FLUSH PRIVILEGES;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ending with FLUSH PRIVILEGES; ensures that all the previous GRANT statements take effect immediately, which is important when testing right after running this script.

Comment thread task.sh
set -euo pipefail

: "${DB_USER:?Environment variable DB_USER is required}"
: "${DB_PASSWORD:?Environment variable DB_PASSWORD is required}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Granting EVENT on ShopDB.* is an extra permission beyond what the description strictly lists, but it fits with your use of --events in mysqldump and is appropriate for backing up event definitions.

Comment thread task.sh
@@ -1 +1,50 @@
#! /bin/bash
#!/usr/bin/env bash
set -euo pipefail

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using set -euo pipefail is good for a cron-oriented script; it ensures that if any mysqldump or mysql command fails, the script will stop instead of silently continuing with a partial backup.

Comment thread task.sh
Comment on lines +8 to +14
FULL_BACKUP_FILE=$(mktemp /tmp/ShopDB_full_backup.XXXXXX.sql)
DATA_BACKUP_FILE=$(mktemp /tmp/ShopDB_data_backup.XXXXXX.sql)

cleanup() {
rm -f "$FULL_BACKUP_FILE" "$DATA_BACKUP_FILE"
}
trap cleanup EXIT

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating temp files with mktemp and cleaning them up with a trap addresses long-term disk usage concerns and is suitable for a nightly job.

Comment thread task.sh
Comment on lines +35 to +40
mysqldump \
-h "$DB_HOST" \
-u"$DB_USER" \
-p"$DB_PASSWORD" \
--single-transaction \
--no-create-info \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using --no-create-info here correctly ensures that only data (not schema) is copied into ShopDBDevelopment, which matches the requirement not to copy schema from production to the development environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants