-
Notifications
You must be signed in to change notification settings - Fork 142
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 Provider Detail view with Yaml and Alerts #2723
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.
Could you please add ProviderDetail and NotificationsTable to index.ts too?
ui/components/AlertsTable.tsx
Outdated
<ul className="event-sources"> | ||
{a?.eventSources?.map((obj: CrossNamespaceObjectRef) => ( | ||
<li className="event-sources" key={obj.name}> | ||
{obj.kind}: {obj.name} |
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.
Should this not (also, I wrote more in the other comment) have a namespace?
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.
I think the comma could be confusing - looks a bit like it's doing some kind of {kind}: {objs.filter(o => o.kind == kind).map(o => o.name).join(', ')}
.
I think I would do {obj.kind}: {obj.namespace}/{obj.name}
I almost said "because that's how it's usually displayed" and then I realised that it's not (if you run flux get all -A
you get the {obj.kind}/{obj.name}
in one column, and namespace in another one, and if you run kubectl port-forward --help
then you'll see that you can also use {obj.kind}/{obj.name}
) - but I can't think of anything better that's still somewhat faithful to the original design.
882c6d9
to
a61582d
Compare
@joshri could you please rebase on main so that I could test search in the events table? Search is supposed to be working in this table, correct? |
bebd062
to
aaee372
Compare
fc24381
to
6a63333
Compare
Closes #2566
Closes #2567
Closes #2568
NotificationsTable
are now clickable links to the newProviderPage
pageuseGetObject
, and passes it down to the newProviderDetail
containingYamlView
andAlertsTable
tabs.AlertsTable
takes the Alerts from an individual provider using the newuseListAlerts
as rows. This hook grabs all Alerts and filters them byproviderRef
to grab the correct ones for the table.Alert
as an extension ofFluxObject
.YamlView
for a while - a 5px blank spot was reserved on the right side of the view, likely for a scrollbar. I removed horizontal scrolling on the view (it already pre-wraps), and extended the header all the way across.The Events tab is impossible at the moment due to the flux notification-controller not recording kubernetes events. There is a remaining issue in the notification epic for a special display where no notifications are visible, and I believe that there were plans to add alert yaml on hover but I don't see an issue for it.
Clickable providers:
Alerts Table:
Provider yaml: