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
Bug 1802034: Silenced alerts should not show up in the Dashboards page of the console [4.5] #4539
Bug 1802034: Silenced alerts should not show up in the Dashboards page of the console [4.5] #4539
Conversation
@dtaylor113: This pull request references Bugzilla bug 1802034, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@dtaylor113: This pull request references Bugzilla bug 1802034, which is valid. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
550e92e
to
db7e670
Compare
db7e670
to
f4a89ee
Compare
/retest |
1 similar comment
/retest |
@@ -104,7 +92,7 @@ export const withDashboardResources = <P extends DashboardItemProps>( | |||
urlResultChanged || | |||
queryResultChanged || | |||
k8sResourcesChanged || | |||
(this.watchingAlerts && alertsResultChanged) || | |||
alertsResultChanged || |
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.
this would cause unnecessary card rerendering even if it does not need to watch alerts.
I'd like to get rid of this HOC in the future - I'd prefer to connect the specific cards to redux/notificationAlerts instead
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.
Good point @rawagner. I could restore the watchAlerts
functions/vars, minus the polling, in order to keep the watchingAlerts
functionality in place to avoid unnecessary card rerendering (although in my limited experience I don't see alertsResultChanges that often), or do you feel the best solution is to connect the specific cards to redux/notificationAlerts in the future?
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.
restoring the function would be fine
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.
Hi @rawagner, is the 👍 for restoring watchAlerts or waiting to connect specific cards to redux/notificationAlerts in the future?
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 can go with restoring the function for now. I will start getting rid of this HOC once #3443 is merged
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.
Hi @rawagner, restored watchAlerts
functionality. PTAL
…tus card to use notificationAlerts from notificationDrawer alert polling
f4a89ee
to
cb3ac2d
Compare
@@ -234,7 +229,8 @@ export type DashboardItemProps = { | |||
stopWatchAlerts: StopWatchAlerts; | |||
urlResults: RequestMap<any>; | |||
prometheusResults: RequestMap<PrometheusResponse>; | |||
alertsResults: RequestMap<PrometheusRulesResponse>; | |||
alertsResults: RequestMap<PrometheusRulesResponse>; // TODO: remove once noobaa-storage-plugin, ceph-storage-plugin, and metal3-plugin status cards have been updated to switch over to alertNotifications, see https://github.com/openshift/console/pull/4539 | |||
notificationAlerts: any; |
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.
There's already a better type in https://github.com/openshift/console/blob/master/frontend/public/components/notification-drawer.tsx#L325 . I think we should move it to reducers/ui.ts
and reuse it in notification-drawer.tsx
and with-dashboard-resources
.
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.
Hi @rawagner, created/moved, pls see last commit for changes -thanks
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtaylor113, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
10 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
This PR addresses hiding silenced alerts in the cluster dashboard
This PR removes alert watching/polling from Dashboards, and switches to use notificationAlerts (non-silenced, firing alerts) from the global Notification Drawer alerts & silences polling.
Now Silencing an Alert hides it from Cluster Dashboard:
Note: this PR affects the following plug-ins which will need to be updated to use the new
notificationAlerts
similar to how frontend/public/components/dashboard/dashboards-page/cluster-dashboard/status-card.tsx has been updated: