-
Notifications
You must be signed in to change notification settings - Fork 124
Add ErrorResponse #743
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
Add ErrorResponse #743
Conversation
🦋 Changeset detectedLatest commit: a7e980a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR 💥 An error occurred when fetching the changed packages and changesets in this PR |
protobufs/livekit_rtc.proto
Outdated
| message ErrorResponse { | ||
| uint32 request_id = 1; | ||
| // http error code | ||
| uint32 code = 2; |
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.
Does this need to be http error code? Can everything be mapped to this?
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.
doesn't need to be! Just wanted to give users a familiar set of codes to work with.
What would you suggest as an alternative?
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.
If it can be mapped, fine. Too often I have found that it ends up being an exercise is trying to decide between a couple of different options. If we can do enum, that will be great.
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 see. Do you think we need anything beyond: a default/unspecified error, a not_found and a not_allowed error?
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.
Probably not. But things always have a way of expanding. We can see close reason/ disconnect reason. It has grown to a few. This could also grow. It may never, but just leaving the possibility open.
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.
updated the proto file to use a Reason enum instead of the http error code
davidzhao
left a comment
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.
lgtm
skipping one id in
UpdateParticipantMetadatain order not to interfere with #733.Will wait for #733 to be merged before merging this.