Add label information to host summary response#5573
Conversation
gillespi314
changed the title
| "offline_count": 141, | ||
| "mia_count": 0, | ||
| "new_count": 0, | ||
| "all_linux_count": 1204, |
There was a problem hiding this comment.
Q regarding platform query argument of GET /api/v1/fleet/host_summary:
- Couldn't the
all_linux_countbe deduced by callingGET /api/v1/fleet/host_summary?platform=linux? - Does the
all_linux_countmake sense on a call likeGET /api/v1/fleet/host_summary?platform=darwin?
There was a problem hiding this comment.
@lucasmrod We'd like to get all counts with a single request. While we could deduce the count using the platform query, it would require multiple requests.
There was a problem hiding this comment.
Makes sense, it's just that now the API arguments and return values are starting to cause confusion.
There was a problem hiding this comment.
@lucasmrod, just checking to see if you are ok with merging this PR in its current form or do you feel that there is more to discuss?
There was a problem hiding this comment.
it's just that now the API arguments and return values are starting to cause confusion
Agreed. Looking at the responses, I'm not sure that we need the platform argument any longer, since as far as I can see no additional information is provided when including that argument. With that in mind, when this is implemented on the frontend I expect we'll be able to make a single request to host-summary and not make additional requests with the platform argument when a platform is selected. @gillespi314 Does that sound right?
There was a problem hiding this comment.
just checking to see if you are ok with merging this PR in its current form or do you feel that there is more to discuss?
Yes, approved.
I can see the value of the all_linux_count. Basically to avoid having the caller (UI) implement heuristics to understand what a "linux" host is and then count (that is, check the returned platforms, e.g. "redhat", "debian", "linux" string, etc.). This way, that thing is defined by the backend only.
| } | ||
|
|
||
| // TODO: should query for "All linux" label be updated to use `platform` from `os_version` table | ||
| // so that the label tracks the way platforms are handled here in the host summary? |
There was a problem hiding this comment.
As discussed in Slack, +1 to create an issue to fix this.
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM overall, left a question around the platform query argument and the all_linux_count return field.
Codecov Report
@@ Coverage Diff @@
## main #5573 +/- ##
=======================================
Coverage 58.17% 58.18%
=======================================
Files 357 358 +1
Lines 33008 33052 +44
=======================================
+ Hits 19204 19230 +26
- Misses 11874 11892 +18
Partials 1930 1930
Continue to review full report at Codecov.
|
|
Editor pass completed on: |
* Editor pass - Add label information to host summary response Editor pass for: - #5573 * Update issue-4738-host-summary
6 participants
Issue #4738
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/and/ororbit/changes/).