Closed mishaschwartz closed 11 months ago
Oh, I only noticed this PR now. Great ! More harmonization with JupyterHub {username}
.
Question about the DB migration, if it encounters existing username with uppercase and try to convert to a lowercase version, will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?
What if the conversion clashes with existing lowercase version?
@tlvu
will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?
It won't display it (in the current state).
That could be a good reason to keep the for user in session.execute(sa.select(users)):
approach in the migration script. A LOGGER.warning
could be employed in the loop to clearly indicate it to the admin within pavics-compose logs -f magpie
.
Warning would be used only if user.user_name != user.user_name.lower()
to avoid being unnecessarily noisy.
What if the conversion clashes with existing lowercase version?
The migration will purposely crash. Magpie will not be able to boot, and therefore the rest of the stack also. It would be up to the admin to investigate and decide how they want to remedy the situation (inform the user of a rename, delete, etc.).
What if the conversion clashes with existing lowercase version?
The migration will purposely crash. Magpie will not be able to boot, and therefore the rest of the stack also. It would be up to the admin to investigate and decide how they want to remedy the situation (inform the user of a rename, delete, etc.).
I assume when Magpie crashes, it will display/log the conflicting existing username causing the crash and the matching uppercase version?
I assume when Magpie crashes, it will display/log the conflicting existing username causing the crash and the matching uppercase version?
Magpie will report the error to boot after the configured number of DB connect/migration retries (default 5).
I assume sqlalchemy
will report some kind of error being unable to update the value due to the "no duplicate" index rule.
However, the migration script itself could provide a log call to be more explicit about the known potential issue and how to address it.
@tlvu @fmigneault
Since the case insensitive unique index is created first (before existing usernames are converted to lowercase). Alembic will report an error indicating which values are in conflict before any conversion happens.
This will give the user a good idea of which names are in conflict and they can update the values manually. I've added a comment in the migration script as well with this information and added additional information to the error message.
Since the case insensitive unique index is created first (before existing usernames are converted to lowercase). Alembic will report an error indicating which values are in conflict before any conversion happens.
This will give the user a good idea of which names are in conflict and they can update the values manually. I've added a comment in the migration script as well with this information and added additional information to the error message.
@mishaschwartz Thanks for logging the error when migration fails.
will it display which users were converted so the node admin can notify the users impacted they have to now use lowercase to login?
It won't display it (in the current state). That could be a good reason to keep the
for user in session.execute(sa.select(users)):
approach in the migration script. ALOGGER.warning
could be employed in the loop to clearly indicate it to the admin withinpavics-compose logs -f magpie
. Warning would be used only ifuser.user_name != user.user_name.lower()
to avoid being unnecessarily noisy.
When migration succeeds, please also log which users were migrated so the node admin can notify those users to use lowercase instead of uppercase.
When migration succeeds, please also log which users were migrated so the node admin can notify those users to use lowercase instead of uppercase.
Ok sounds good. Done now
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
436c7f9
) 80.92% compared to head (427e724
) 80.92%. Report is 9 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Ziggurat foundations assumes that a
User
will not have auser_name
that differs from another only in terms of case. The simplest way to enforce this is to ensure that alluser_name
values are lowercase.Previously, this was not enforced so we could create two
User
which could not be differentiated properly.This change includes a database migration that will convert all
user_name
that contain uppercase characters to lowercase. This may cause a database conflict if there are twouser_name
values that differ only in terms of case. For example "Test" and "test". If this occurs, please manually update thoseuser_name
values to no longer conflict and try the migration again.This also prevents new users from being created that contain whitespace.
Replaces #596 Resolves #595