Add GetServers parameterized search for Connect grpc#15934
Add GetServers parameterized search for Connect grpc#15934
Conversation
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
The overall approach looks fine, I left some comments regarding the details of the implementation.
Perhaps the most important thing is preparing the accompanying webapps PR which updates the protos on the frontend.
…ithub.com/gravitational/teleport into michaelmyers/connect/add-parameter-search
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Looks fine but I'd strongly suggest refactoring that web server part to use the new function and adding some tests. Leaving a preemptive approve since I might not have time to look at the PR again before I'm off.
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Looks stellar now, good job.
| // index 0 is fieldName and index 1 is direction. | ||
| // If a direction is not set, or is not recognized, it defaults to ASC. | ||
| func GetSortByFromString(sortStr string) (sortBy SortBy) { | ||
| if sortStr != "" { |
There was a problem hiding this comment.
I would invert this check, so that most of the code is left-aligned:
if sortStr == "" {
return sortBy
}
vals := strings.Split(...
...
| @@ -0,0 +1,22 @@ | |||
| package types | |||
There was a problem hiding this comment.
Is types the best package for this? This is a pretty low-level commonly imported package. Maybe there's a better place for it?
There was a problem hiding this comment.
This was our first best guess at where to put it. I'm open to suggestions, not quite sure which context suites it best.
…ithub.com/gravitational/teleport into michaelmyers/connect/add-parameter-search
|
Closing this because the changes are coming in my larger PR for Connect access requests. |
6 participants
This PR will change Connect's grpc endpoint of
ListServersto two different endpoints,GetAllServersandGetServers.ListServersdid and is just a name change. The reason we need to keep this functionality is that it will pull ALL servers at cluster init (in chunks of 1000). It calls the same thing astsh/tctlhere. We shouldn't add pagination and sorting to this endpoint, especially since the web server already has it.ListResources. This is how we will do server side pagination/sorting/searching (including advanced) search. For now, Connect will only use this endpoint during access requests.I left a comment around getting the authClient for ListResources and will remove it before merging but wanted to leave it marked for feedback. Was trying to access the same thing as
cltfound here in the webui, although I'm a bit unsure if thats the best. hacked around on that part tbh.webapps related PR here