-
Notifications
You must be signed in to change notification settings - Fork 289
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 human friendly error messages #1154
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.
I see some problems with the approach:
- we're losing context - we don't know which channel is invalid, or which channel the bot should join. These errors are user friendly, but unfortunately not helpful.
- it will be hard to maintain, as some of the errors are wrapped even 2 times.
- we still log less user friendly messages.
While the idea of error mapping of course totally makes sense, I'd suggest doing it in different way:
- map the errors in corresponding Bots (like bot/socketslack.go, bot/discord.go)
- avoid string comparison if possible: if discord/mattermost/slack packages export some errors, use them with
errors.Is(...)
. Of course if that's not possible, then string comparison is our last resort. - add context like channel name and communication group name.
- avoid string comparison if possible: if discord/mattermost/slack packages export some errors, use them with
- these errors will be still logged, wrapped and reported as they were in main. So user checking logs will be able to understand the issue, with all the details.
What do you think? Thanks!
Hi @pkosiec here is what I did; SlackThey don't have error model for our cases, so I couldn't use them, used error string instead DiscordI have used their error model and re-mapped error with additional context MattermostWe don't need to do anything for mattermost since they already have error description, and they are human-readable. |
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.
👌 Looks good to me! Didn't test it but based on the screenshots I see that it works great.
212bda1
to
b7f7bc3
Compare
Description
Changes proposed in this pull request:
Here are the screenshots for testing results for different scenarios for Slack, Discord, and Mattermost
Slack
Discord
Testing
You can create an instance on Botkube Cloud Dev, and provide wrong creds, wrong channel, or not invited channel for each platform. You need to see human-friendly messages as shown above.
You need to run botkube deployment via
go run ...
on this branch after exportingRelated issue(s)
https://github.com/kubeshop/botkube-cloud/issues/520