-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Show Count of Agent Policies on Integration Details #86916
[Fleet] Show Count of Agent Policies on Integration Details #86916
Conversation
…add-used-by-counts
Pinging @elastic/ingest-management (Team:Ingest Management) |
@@ -190,12 +224,27 @@ describe('when on integration detail', () => { | |||
}); | |||
}); | |||
|
|||
interface MockedApi { | |||
interface MockedApi< |
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.
Some of this here is refactoring which will facilitate breaking this out to a separate set of test utilities so that this approach can be used with other test areas of Fleet.
The mocked api interface could now also return the set of functions that are used to fulfill the API calls, which allow each test case to adjust them (if necessary) before rendering the UI. An example of this can be seen above on line 69-70
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 appreciate the tests. Can we wait to merge this until we've had a chance to discuss the goal of the new endpoint and any alternative names / locations?
Alternatively, we could merge this knowing it's subject to change on further review
@jfsiii sure, we can wait on this until a broader discussion can be had. Thanks for looking at it. |
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.
Code looks good, and works as expected. It so nice to start to see some frontend tests in Fleet :)
We should probably add this endpoint to our Open API spec
@@ -29,6 +29,7 @@ export const EPM_API_ROUTES = { | |||
DELETE_PATTERN: EPM_PACKAGES_ONE, | |||
FILEPATH_PATTERN: `${EPM_PACKAGES_FILE}/{filePath*}`, | |||
CATEGORIES_PATTERN: `${EPM_API_ROOT}/categories`, | |||
SUMMARY_PATTERN: `${EPM_PACKAGES_MANY}/{pkgName}/summary`, |
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.
nitpick maybe change the name to something a little less generic /stats
/usages
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.
👍 Let's go with /stats
since @jen-huang also suggested that for the name
…add-used-by-counts # Conflicts: # x-pack/plugins/fleet/server/routes/epm/handlers.ts
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.
Functionally, seems all good.
Noted a few places that could be cleaned up
x-pack/plugins/fleet/public/applications/fleet/sections/epm/screens/detail/index.test.tsx
Outdated
Show resolved
Hide resolved
@@ -29,6 +29,7 @@ export const EPM_API_ROUTES = { | |||
DELETE_PATTERN: EPM_PACKAGES_ONE, | |||
FILEPATH_PATTERN: `${EPM_PACKAGES_FILE}/{filePath*}`, | |||
CATEGORIES_PATTERN: `${EPM_API_ROOT}/categories`, | |||
STATS_PATTERN: `${EPM_PACKAGES_MANY}/{pkgName}/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.
This seems to be called summary elsewhere. Perhaps we should make the var names consistent, to mentally correlate them a bit better.
If the route already exists and is /stats
, I can see giving the route's var name the same as the route itself. But then it's switched to called summary on the layer right outside that. Consider naming the pattern var name summary as well. Reduces the mental context switch to just here, next to the route, which is a little more concrete to reason about.
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 have renamed everything else *Stats*
in order keep consistent with this decision here 😄 . Thanks for highlighting that - "me culpa" 😞
updated_at: '2020-12-22T21:28:05.383Z', | ||
version: 'WzE1NTAsMV0=', | ||
score: 0, | ||
}, |
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.
Is this much mocking and setup really necessary for testing paging?
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.
yes and no. Some packge policies (integration policies) can have the same Agent Policy id (with the exception of Endpoint, every other integration can be added to agent policies multiple times). The multiples assigned to the same agent policy is to ensure the counts remain accurate (it should just be counted once)
let savedObjectsResponse: typeof savedObjects; | ||
|
||
switch (page) { | ||
case 1: |
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.
so in this mock.. if it's page 1, you return index 1, the second element.
If it's page 2, you return indices 1,2,3?
It's not clear why you're doing this. It also doesn't seem like any of these scenarios are necessary in the tests below.
unless this is strictly for making sure the getPackageUsageSummary
call doesn't error, but then I think this could all be simpler
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.
Good catch. This here (below) should be savedObjects[0]
.
the idea is:
- page 1 - return first item in array
- page 2 - return every other array item after the first one
Doing this ensures that paging through the data works and that it de-dups the packages that are assigned to the same agent policy
Ok. I think I got all of @pzl comment in. Ready for another round of 👀 |
@elasticmachine merge upstream |
// in order to not cause a circular dependency (package policy service imports from this module) | ||
const packagePolicies = await savedObjectsClient.find<PackagePolicySOAttributes>({ | ||
type: PACKAGE_POLICY_SAVED_OBJECT_TYPE, | ||
perPage: 1000, |
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.
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 see some lingering references to *Summary
(mostly in fleet/sections/epm/screens/detail/index.test.tsx
). I'd like to remove them before merging, but we can merge and follow up.
Thanks @jfsiii . I am fixing those now 🙂 |
…nts' into task/fleet-86369-add-used-by-counts
…add-used-by-counts
…add-used-by-counts # Conflicts: # x-pack/plugins/fleet/server/services/epm/packages/get.ts
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
/api/fleet/epm/packages/{pkgName}/stats
which returns summary of agent policiesCloses #86369
Checklist