-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix: limit number of resources in appset status #24690
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
fix: limit number of resources in appset status #24690
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this 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.
Thank you! The changes looks good to me. Few points:
- Can you please add unit test here?
- The lint check is failing that needs to be fixed.
e242d02 to
5ac46ea
Compare
|
Thanks for looking at PR @nitishfy !
|
5ac46ea to
99fed87
Compare
|
Should we have a condition to indicate the truncation to the user/UI? |
cmd/argocd-applicationset-controller/commands/applicationset_controller.go
Outdated
Show resolved
Hide resolved
|
Thanks for review @crenshaw-dev !
I was thinking about too while writing PR description. Added |
e7e4f16 to
e45fcc4
Compare
| // Resources is a list of Applications resources managed by this application set. | ||
| Resources []ResourceStatus `json:"resources,omitempty" protobuf:"bytes,3,opt,name=resources"` | ||
| Resources []ResourceStatus `json:"resources,omitempty" protobuf:"bytes,3,opt,name=resources"` | ||
| ResourcesCount int64 `json:"resourcesCount,omitempty" protobuf:"varint,4,opt,name=resourcesCount"` |
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.
Probably wants a doc string so the future UI dev knows how to use this.
Are you hoping to cherry-pick the change back? I think for 3.2 it would be fine, but adding a CRD field further back feels maybe not great.
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, you are right. What do you think about adding resourcesCount in a separate PR? The UI functionality is not ready yet, so we are not loosing anything without resourcesCount. We can cherry-pick first PR into previous release and resourcesCount can go only to master branch.
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've moved change to a separate commit: d5e690f . If you are fine with it, I will create new PR as soon as this one merged.
e45fcc4 to
74b3182
Compare
Signed-off-by: Alexander Matyushentsev <[email protected]>
74b3182 to
98dcce3
Compare
Signed-off-by: Alexander Matyushentsev <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #24690 +/- ##
==========================================
+ Coverage 60.46% 60.52% +0.06%
==========================================
Files 350 350
Lines 60187 60196 +9
==========================================
+ Hits 36390 36434 +44
+ Misses 20859 20827 -32
+ Partials 2938 2935 -3 ☔ View full report in Codecov by Sentry. |
|
❌ Cherry-pick failed for 3.1. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 3.0. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 3.2. Please check the workflow logs for details. |
|
❌ Cherry-pick failed for 2.14. Please check the workflow logs for details. |
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Signed-off-by: Alexander Matyushentsev <[email protected]>
Closes #24689
The
applicationSet.status.resourcesis meant for appset visualization in a future UI/CLI. However it limits the number of applications that can be managed by a single appset.The desire to build UI should not limit core functionality of application set. From experience of building application tree visualization we know that 1000+ resources is already hard to scale. So in this PR I'm proposing to introduce
ARGOCD_APPLICATIONSET_CONTROLLER_MAX_RESOURCES_STATUS_COUNTthat limits number of resources stored in status (5k by default). So this way appset can generate any number of apps and in a future UI we can add a warning that only first 5k is shown.