- Notifications
You must be signed in to change notification settings - Fork906
fix: rewrite login type migrations#16978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for sorting this out 👍
Uh oh!
There was an error while loading.Please reload this page.
| -- The original disregarded the warning in create_migration.sh. | ||
| -- As a result, it was not possible to insert a user with the "none" login type | ||
| -- in a migration. | ||
| -- The new implementation makes the same changes as the original, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: this comment should rather just going into the PR description, and the original argument should be copied over as well. Linking to a GH commit might not always work, or be difficult to find once the comment is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm going to keep a trimmed version of the comment in the migration if its all the same, but I will update the PR description with some more detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sounds good; for consistency should it not go in both files then?
a3f6308 intomainUh oh!
There was an error while loading.Please reload this page.
When trying to addsystem users, we discovered an issue in two migrations that added values to the login_type enum.
After someconsideration, we decided to retroactively correct them.