-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: add server side apps pagination(#14947) #22444
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
base: master
Are you sure you want to change the base?
Conversation
❌ Preview Environment undeployed from BunnyshellAvailable commands (reply to this comment):
|
3a39083 to
2bf3a0c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22444 +/- ##
==========================================
- Coverage 60.22% 59.97% -0.25%
==========================================
Files 346 347 +1
Lines 59197 59397 +200
==========================================
- Hits 35653 35626 -27
- Misses 20686 20909 +223
- Partials 2858 2862 +4 ☔ View full report in Codecov by Sentry. |
2bf3a0c to
58e7d03
Compare
58e7d03 to
6306337
Compare
|
PTAL @alexmt |
6306337 to
3b80d3b
Compare
c43997a to
44322fa
Compare
9deaaaa to
228710c
Compare
linghaoSu
left a comment
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
PS: If you're not interested in making modifications in the frontend/CLI, I'm happy to take this on.
Great! I know nothing about UI stack, please feel free to do it. |
228710c to
9ebb492
Compare
|
@linghaoSu Any idea on when you might get around to the frontend implementation? Eagerly awaiting this feature :) |
68bfe23 to
786d15e
Compare
786d15e to
e04eac5
Compare
|
Love, it, really appreciate if you can release it, our ArgoCD home page has been stuck there for months because the browser cannot handle 5000+ apps in a single response. |
| metav1.ListMeta `json:"metadata" protobuf:"bytes,1,opt,name=metadata"` | ||
| Items []Application `json:"items" protobuf:"bytes,2,rep,name=items"` | ||
| Items []Application `json:"items" protobuf:"bytes,2,rep,name=items"` | ||
| Stats ApplicationListStats `json:"stats,omitempty" protobuf:"bytes,3,opt,name=stats"` |
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.
The list stats should be part of an ApplicationListResponse object specific to the server/application/application.proto.
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.
Sure, that sounds more reasonable, but it might be a problem that a breaking change was introduced. Maybe we should add a new method instead of modifying the return type of the existing one?
server/application/application.proto
Outdated
| // the application name to start from (app with min name is included in response) | ||
| optional string minName = 9; | ||
| // the application name to end at (app with max name is included in response) | ||
| optional string maxName = 10; | ||
| // the repos filter, ignore if the field repo was defined | ||
| repeated string repos = 11; | ||
| // the clusters filter | ||
| repeated string clusters = 12; | ||
| // the namespaces filter | ||
| repeated string namespaces = 13; | ||
| // the auth sync filter | ||
| optional bool autoSyncEnabled = 14; | ||
| // the sync status filter | ||
| repeated string syncStatuses = 15; | ||
| // the health status filter | ||
| repeated string healthStatuses = 16; | ||
| // search | ||
| optional string search = 17; | ||
| // offset | ||
| optional int64 offset = 18; | ||
| // limit | ||
| optional int64 limit = 19; | ||
| // sort by enum=name;created-at;synchronized | ||
| optional ApplicationSortBy sortBy = 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.
These should be wrapped in a ListOptions object. Since some fields are duplicated, the old fields should probably be flagged as deprecated.
server/application/application.go
Outdated
| return q.Repos | ||
| } | ||
|
|
||
| func buildFilter(q application.ApplicationQuery) argo.Filter { |
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.
Move to its own utils package.
server/application/application.go
Outdated
| for _, project := range getProjectsFromApplicationQuery(*q) { | ||
| projects[project] = true | ||
| } | ||
| filter := buildFilter(*q) |
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.
The watch/get code is different that the list.
Perhaps they should 1) Not use the same ApplicationQuery object and 2) not have any of the listing logic.
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.
Sorry, I’m not quite sure I understand. Could you clarify the difference between watch and get in this context? From what I see, all the filters seem applicable to the watch logic as well, so that the UI only receives events related to the applications it’s interested in.
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.
@agaudreault can you help us review this change? thanks
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.
@muma378 can we find another one to review this PR?
|
Hi @agaudreault, can you tell us when this PR will be released ? |
cea5e12 to
9516f5f
Compare
Signed-off-by: yang.xiao <[email protected]>
9516f5f to
cb7d492
Compare
Signed-off-by: yang.xiao <[email protected]>
cb7d492 to
8bb03f0
Compare
|
@crenshaw-dev Wondering if we can get some attention here. I think many of us are eagerly awaiting this feature to be implemented. Thanks! |
|
For folks awaiting this feature, I have reached out in one of the CNCF Slack channels for ArgoCD to try and get some attention |
|
Unfortunately development is not ongoing. This was the response I received:
|
KEP: #17222
Closes #14947
Checklist: