Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Make admin recovery to work without emails #1419 #1750
feat: Make admin recovery to work without emails #1419 #1750
Changes from all commits
a750020
c6f3d98
a6be25c
3492ca5
45ec59b
e8c943f
142d4dd
db4b63f
823050a
f57f1bb
99441b1
58f696d
5272633
5eaedc4
1fc3eb4
48bcb38
d19eede
08a6d1f
462df89
6810666
053da6b
0c72018
b3ff59f
9bfe61e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You can add foreign keys in sqlite like so:
kratos/persistence/sql/migrations/templates/20210410175418_network.up.fizz
Line 4 in 4bec9ef
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.
Sorry missed that comment. I'll try to fix it today
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.
@aeneasr if I add a column this way the migrations don't generate properly. The identity_id column is added i one of the first migrations and then is being removed after some temp table is generated and data is copied there. This causes tests to fail since the identity_id column doesn't exist
"sqlite create: table identity_recovery_tokens has no column named identity_id"
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.
Could you share the full up/down migration?
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.
@aeneasr 45ec59b
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.
Did it help? :)
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.
sorry @aeneasr no it didn't. I still get "sqlite create: table identity_recovery_tokens has no column named identity_id" . The only change that wasn't in the pr is in this file: 20210410175418000063_network.sqlite3.down.sql, but it doesn't matter.
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.
Ok, I'll take a look
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 just checked out your branch, ran
rm -rf persistence/sql/migrations/sql/* && make migrations-render-replace
and it worked. Not sure what's going on on your end?Assuming tests pass, is this PR complete in your opinion?
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.
make migrations-render-replace
works, but tests fail after that the same way. After regenerating migrations the only difference between my pr and local changes is the file I mentioned in the previous comment.As for the PR I would say it's finished.