-
Notifications
You must be signed in to change notification settings - Fork 14
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
Added alert banner in netobserv page #286
Added alert banner in netobserv page #286
Conversation
<AlertFetcher> | ||
<NetflowTraffic forcedFilters={null} /> | ||
</AlertFetcher> |
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 wonder if this should be here or inside netflow-traffic.tsx
. The tabs will not show the alerts as is. Is it the expected behavior ?
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 was intended to not have this in the tabs, I thought it would not fit in the tab view.
You think we should add it there too?
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 don't know the console behavior in this case. May @andrew-ronaldson has a better knowledge ?
setAlerts( | ||
result.data.groups.flatMap(group => { | ||
return group.rules | ||
.filter(rule => !!rule.labels.namespace && rule.labels.namespace == 'netobserv' && rule.state == 'firing') |
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.
'netobserv' should come from config here ?
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 added configuration through the frontend config endpoint, thanks!
export const AlertFetcher: React.FC<AlertFetcherProps> = ({ children }) => { | ||
const [alerts, setAlerts] = React.useState<AlertsRule[]>([]); | ||
React.useEffect(() => { | ||
getAlerts().then(result => { |
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 should also catch errors and at least log it in the console
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.
Done, thanks!
web/src/api/alert.ts
Outdated
rules: AlertsRule[]; | ||
} | ||
|
||
export interface AlertsRule { |
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 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.
Done, thanks!
Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com>
Co-authored-by: Julien Pinsonneau <91894519+jpinsonneau@users.noreply.github.com>
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, thanks @OlivierCazade
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade 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 |
Request monitoring API and display banners if netobserv alerts are firing.