Revert: Grant migrator privileges even when the user already exists#8313
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
combs-a
left a comment
There was a problem hiding this comment.
I wrote these testing instructions, but they may need to be added onto if anyone spots any additional coverage needed!
New database:
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 initial database setup screen.
Previously existing database (used pre v7.12):
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 login page.
New database worked as expected.

Was able to create a new database with no issues, but ran into an issue with the Specify app user when attempting to open an old database. Used the same database as I did in the originally reported issue.
|
Noting this here--the regex also needs to be checked; there is an issue with not forming the database names correctly. Had a DB with an underscore in it and so it attempted to grant permissions to Not quite in the scope of just this PR but should be fixed. |
… to preexisting appuser
There was a problem hiding this comment.
New database:
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 initial database setup screen.
Previously existing database (used pre v7.12):
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 login page.
New database works as expected, and as long as I destroy the database volume when rebuilding the containers (docker compose down -v), using an existing database works as well.
If I don't destroy the volume, mariadb will not source the seed-database/*.sql files, and in this case I have to add
CREATE DATABASE IF NOT EXISTS `...`to the start of the .sql file, So I'm not sure if that is the intended behavior. If I omit the -v, then I do have migration errors, because the existing database is not loaded by the mariadb container. If this is intended behavior I can approve.
|
I haven't run into that issue, I'll look into it! |
|
I also tested the new regex formatting for database names with underscores in them, and that worked as well! There were no weird escape characters getting misinterpreted as part of the database name. |
combs-a
left a comment
There was a problem hiding this comment.
New database:
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 initial database setup screen.
Pre-existing database:
- Check the
specify7container logs and make sure there are no migration errors. - Make sure that it's opening the Specify 7 login page.
Additional checks:
- See that changing the migrator name to master in
.envprints the proper log message. - See that changing the app user name to master in
.envprints the proper log message. - See that changing the app user name to the migrator in
.envprints the proper log message. - See that having an underscore or other characters that commonly need to be escaped does not affect database setup.
I used new_db and cash_prev712 (a pre-existing database) to test the regex, and it worked fine. Other special characters should maybe be tested for completeness, but I think generally other characters are not used in filenames.
Used a mix of pre-existing and new databases to test the name changes as well, and it seemed all good. 👍
rijulpoudel
left a comment
There was a problem hiding this comment.
Tested this locally and everything worked as expected.
New database
- Existing migrator and app users were detected correctly.
- The users received the required privileges on the new database.
- No migration or database permission errors occurred.
- The initial Specify 7 database setup screen opened successfully.
Pre-existing database
- The existing database started without migration or permission errors.
- Migrator and app-user privileges were refreshed and verified.
- The Specify 7 login page opened successfully.
Additional checks
- Using the master credentials for the migrator printed the expected messages and worked correctly.
- Using the master credentials for the app user printed the expected messages and worked correctly.
- Using the migrator credentials for the app user printed the expected messages and worked correctly.
- A database name containing underscores was handled correctly without unwanted escaping.
I saw some non-fatal Schema Config warnings for old tables, but they did not interrupt the migration pipeline or application startup.
Everything looks good to me.
Fixes #8305
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
specify_migrator* andspecify_user* already created in MySQL.MIGRATOR_NAMEandAPP_USER_NAMEin the .env file..envfile so that the DATABASE_NAME is one that does not exist yet.specify7container logs and make sure there are no migration errors..envfile so that the DATABASE_NAME is one that does exist and was not previously opened in v7.12.specify7container logs and make sure there are no migration errors.Additional checks:
.envprints the proper log message..envprints the proper log message..envprints the proper log message.