-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fetch resources using ListUnifiedResources
in the search bar
#41357
Conversation
We shouldn't have added it, `addrWithProtocol` is calculated from other `tsh.App` properties, so we can just do it when needed. This type prevents assigning `listUnifiedResources` response to the type returned from `searchResources`. Also, this change will make setting up mocks and tests simpler.
We will fetch all resources for a cluster in a single request, so it is no longer needed.
Previously we fetched 5 items x 4 resource kinds.
@@ -775,17 +775,13 @@ export function makeKube(source: Kube) { | |||
}; | |||
} | |||
|
|||
export interface App extends TshdApp { |
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.
* | ||
* Always empty for SAML applications. | ||
*/ | ||
export function getAppAddrWithProtocol(source: TshdApp): string { |
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.
It looks like a good candidate to be moved to web/packages/teleterm/src/services/tshd/app.ts
.
The only thing that kind of don't like about web/packages/teleterm/src/services/tshd/app.ts
and web/packages/teleterm/src/services/tshd/gateway.ts
is the need to include app
, gateway
, CliCommand
in the function names. But I don't really know how to work around that in JS which let's you import functions without the "namespace" (unlike for example Go, which would have forced us to do something like app.GetAddrWithProtocol
.
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.
Ah, I wanted to move the function but in the end I forgot about it, done.
// example results in the UI. | ||
limit = 5; | ||
limit = 20; |
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 used to have 5 for each supported resource type, which technically gives 20, yeah.
However, rankResults
is always going to return 10 results max anyway. What do you think about extracting the magic number from rankResults
to a const (maxRankedResults
?) and reusing it 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.
Nice!
In upcoming PRs we are going to allow requesting resources from the 'main resources' page.
This will work by returning resources that the user can request and displaying the "+ Add to Request" button instead of "Connect".
We should support this mechanism in the search bar too, and the first step is converting it to use
ListUnifiedResources
instead of individual calls forGetApps
,GetKubes
, etc.Another benefit is a small performance improvement, sending one request should be faster than four (even concurrent).