-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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: implement filtering on cluster List
API endpoint
#13363
fix: implement filtering on cluster List
API endpoint
#13363
Conversation
c995da6
to
3b5f9c3
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #13363 +/- ##
==========================================
+ Coverage 49.17% 49.24% +0.06%
==========================================
Files 248 248
Lines 42872 42925 +53
==========================================
+ Hits 21084 21139 +55
+ Misses 19687 19680 -7
- Partials 2101 2106 +5
☔ View full report in Codecov by Sentry. |
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 tested the PR and the List call does work with the filer.
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.
@onematchfox would you mind adding a note to the 2.8 upgrade notes about this? Even though it's fixing something that's obviously broken, it is technically a breaking API change. Good to give folks a heads-up.
3eb2f9f
to
2f94493
Compare
@crenshaw-dev, not 100% sure what upgrade notes you are referring to - perhaps because I'm the first "upgrade note" for |
2f94493
to
919eb45
Compare
Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
919eb45
to
bd97f6f
Compare
Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
bd97f6f
to
ef44e03
Compare
@crenshaw-dev Just a reminder that this one is ready for final review |
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.
Thanks, @onematchfox!
* feat: implement filtering on cluster list endpoint Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com> * docs: add upgrade notes Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com> --------- Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
* feat: implement filtering on cluster list endpoint Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com> * docs: add upgrade notes Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com> --------- Signed-off-by: OneMatchFox <878612+onematchfox@users.noreply.github.com>
For known security reasons, the
Get
endpoint on the Clusters service returns a403
rather than a404
if a cluster is not found. This means that in order to determine the non-existence of a cluster API, users need to make use of theList
endpoint (see comment related to workaround on the behaviour on the applications service).The application service implements filtering on List allowing the caller to effectively explicitly request a list consisting of a single application which makes this an effective workaround. However, at present, no form of filtering is applied on the cluster
List
API endpoint despite query parameters being available (see screenshot) below. This means that the same workaround for the clusters service is (extremely) inefficient as all clusters are returned on every call and the caller is left to iterate over all clusters. This PR addresses this issue by implementing filtering on this endpoint using the existing cluster query parameters (in the same manner as the application service.Semi-related to #13000 (see comment here)
Checklist:
Please see Contribution FAQs if you have questions about your pull-request.