-
Notifications
You must be signed in to change notification settings - Fork 338
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
improvement: add do not show bsp errors option #5678
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 the constant tweaks! We will get it right at some point 😅
) | ||
} else { | ||
( | ||
makeLongMessage(message), | ||
List(goToLogs, restartBuildServer, dismiss), | ||
List(goToLogs, restartBuildServer, dismiss, doNotShowErrors), |
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.
List(goToLogs, restartBuildServer, dismiss, doNotShowErrors), | |
List(goToLogs, dismiss, doNotShowErrors), |
I am wondering it even help, most people will probably not restart or click mindlessly on it without looking into logs. We could mention it in the message. "the error might require restating build server"
@@ -62,6 +67,8 @@ class BspErrorHandler( | |||
restartBspServer().ignoreValue | |||
case BspErrorHandler.dismiss => | |||
Future.successful(dismissedErrors.add(message)).ignoreValue | |||
case BspErrorHandler.doNotShowErrors => | |||
Future.successful { doNotShowErrors = true } |
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.
Let's save it to database as in case of other notifications
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.
Don't we want to move this long term to bsp status + doctor anyway? Then this flag won't be needed anyway.
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.
Hmm.... in that case yeah. Are we able to sensibly move it to status? We could add the option to disable for now until this is implemented
@@ -0,0 +1,4 @@ | |||
-- If should show bsp errors | |||
create table show_bsp_errors( |
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.
We could add it to the notifications table, no?
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.
Somehow I wasn't aware we had such a generic mechanism for notifications.
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! Should have mentioned it earlier
bca9601
to
fda848a
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.
LGTM!
Add possibility to stop showing bsp errors.