-
Notifications
You must be signed in to change notification settings - Fork 138
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
chore: upgrade to kratos v13 #2563
Conversation
97e198b
to
966095c
Compare
966095c
to
6e64116
Compare
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 approving but think we should remove / handle comments.
|
||
// we expect the phone number to not exist | ||
if (user instanceof Error) { | ||
console.log("phone doesn't already exist") |
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.
Can we send an OTEL event instead of log?
return | ||
} | ||
|
||
console.error("phone already exist") |
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.
Otel event vs log?
return | ||
} | ||
|
||
// TODO |
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.
Is this really needed? I don't think we will loose site of the fact that we may want to implement email+pw
@@ -51,16 +107,18 @@ kratosRouter.post( | |||
|
|||
const userIdChecked = checkedToUserId(userId) | |||
if (userIdChecked instanceof Error) { | |||
// TODO: log this error as critical to honeycomb |
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.
Then we should probably do it pre-merge or?
@@ -83,6 +141,7 @@ kratosRouter.post( | |||
} | |||
|
|||
if (account instanceof Error) { | |||
// TODO: log this error as critical to honeycomb |
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.
Then we should do it pre-merge
No description provided.