-
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
Make host card overview space aware #113983
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
@elasticmachine merge upstream |
merge conflict between base and head |
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! I defer to you on making the space-aware index logic a helper method or not (cc @machadoum).
loginAndWaitForPage(OVERVIEW_URL); | ||
cy.get( | ||
`${OVERVIEW_RISKY_HOSTS_LINKS} ${OVERVIEW_RISKY_HOSTS_LINKS_WARNING_INNER_PANEL}` | ||
).should('not.exist'); | ||
cy.get(`${OVERVIEW_RISKY_HOSTS_VIEW_DASHBOARD_BUTTON}`).should('be.disabled'); | ||
cy.get(`${OVERVIEW_RISKY_HOSTS_TOTAL_EVENT_COUNT}`).should('have.text', 'Showing: 1 host'); | ||
|
||
changeSpace(testSpaceName); |
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.
A++
export const changeSpace = (space: string) => { | ||
cy.get(`${SPACES_BUTTON}`).click(); | ||
cy.get(getGoToSpaceMenuItem(space)).click(); | ||
cy.get(`[data-test-subj="space-avatar-${space}"]`, { timeout: 120000 }); |
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 code as it is currently written will query the page once, and wait up to 2 minutes for a result; adding an assertion here will allow the cy.get
to be retried if the assertion fails:
cy.get(`[data-test-subj="space-avatar-${space}"]`, { timeout: 120000 }); | |
cy | |
.get(`[data-test-subj="space-avatar-${space}"]`}) | |
.should('exist'); |
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.
More to the point: without the assertion, I think there's a potential race condition between the click/page load and this subsequent cy.get
.
...curity_solution/public/overview/containers/overview_risky_host_links/use_risky_host_links.ts
Outdated
Show resolved
Hide resolved
9dbdf66
to
4e7961d
Compare
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, great work 😊
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* Make host card overview space aware * Add cypress test * Move getHostRiskIndex to helpers * Fix cypress test Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
@@ -67,6 +68,7 @@ export interface StartPlugins { | |||
timelines: TimelinesUIStart; | |||
uiActions: UiActionsStart; | |||
ml?: MlPluginStart; | |||
spaces: SpacesPluginStart; |
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.
@nkhristinin I think this should be optional. Just came across this as a conflict to my PR -> #112478
spaces: SpacesPluginStart; | |
spaces?: SpacesPluginStart; |
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.
CC: @rylnd @ecezalp just FYI since I saw you both reviewed this. The security solution has the spaces plugin listed as optional. We should not be assuming this is always present. This could have lead to some not-so-great bugs.
kibana/x-pack/plugins/security_solution/kibana.json
Lines 29 to 43 in a0b55b3
"optionalPlugins": [ | |
"encryptedSavedObjects", | |
"fleet", | |
"ml", | |
"dashboard", | |
"newsfeed", | |
"security", | |
"spaces", | |
"usageCollection", | |
"lens", | |
"lists", | |
"home", | |
"telemetry", | |
"telemetryManagementSection" | |
], |
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 for fixing it in your PR!
…es by `id` (#112478) * added outcome to backend routes * adds so resolved property alias_target_id to response * adds UI portion * working URL redirect on aliasMatch - todo -> update rule details page refresh button to use SO resolve. * cleanup * fix integration tests * fix jest tests * cleanup types * fix eslint.. I think vs code formatted this * WIP - undo me, working index.test.ts function * WIP - also undo me, probably * working test for aliasMatch, need to add test for outcome = conflict * add conflict callout when SO resolve yields conflict outcome * code cleanup * fix type issues * small cleanup, fix jest test after undoing changes for getFailingRuleStatus * cleanup tests * add alias_target_id to response validation too * unit test changes * update tests again * add all dependencies to useEffect and prefer useMemo * add type cast * adds integration tests for different outcomes after mocking a migrated rule leading to an aliasMatch and a migrated rule + accidental inserted rule to lead to a conflict. Also removes the outcome property if it is an exactMatch * remove unused import * fix test * functional WIP * cleanup * cleanup * finishing touches to address PR review comments * remove console.error * fix bug where spaces was not typed correctly in the plugin start method here #113983
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
* Make host card overview space aware * Add cypress test * Move getHostRiskIndex to helpers * Fix cypress test Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Khristinin Nikita <[email protected]>
…ng rules by `id` (#112478) (#114683) * [Security Solution] [Platform] Utilize SO resolve api for reading rules by `id` (#112478) * added outcome to backend routes * adds so resolved property alias_target_id to response * adds UI portion * working URL redirect on aliasMatch - todo -> update rule details page refresh button to use SO resolve. * cleanup * fix integration tests * fix jest tests * cleanup types * fix eslint.. I think vs code formatted this * WIP - undo me, working index.test.ts function * WIP - also undo me, probably * working test for aliasMatch, need to add test for outcome = conflict * add conflict callout when SO resolve yields conflict outcome * code cleanup * fix type issues * small cleanup, fix jest test after undoing changes for getFailingRuleStatus * cleanup tests * add alias_target_id to response validation too * unit test changes * update tests again * add all dependencies to useEffect and prefer useMemo * add type cast * adds integration tests for different outcomes after mocking a migrated rule leading to an aliasMatch and a migrated rule + accidental inserted rule to lead to a conflict. Also removes the outcome property if it is an exactMatch * remove unused import * fix test * functional WIP * cleanup * cleanup * finishing touches to address PR review comments * remove console.error * fix bug where spaces was not typed correctly in the plugin start method here #113983 # Conflicts: # x-pack/plugins/security_solution/public/overview/containers/overview_risky_host_links/use_hosts_risk_score.ts # x-pack/plugins/security_solution/public/types.ts * fix eslint * skip 8.0 integration tests for 7.16
Summary
Host overview card now is using space value to make requests to this index:
ml_host_risk_score_latest_${SPACE}
How to test
Create ml_host_risk_score_latest_default index
Add data to ml_host_risk_score_latest_default index
Last year
rangeChecklist
Delete any items that are not applicable to this PR.
For maintainers