Skip to content
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

[Security Solution] Rule installation/upgrade error handling fixes #159823

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Jun 15, 2023

Related to: #158450

Summary

Various fixes to the rule installation and upgrade workflows:

  • Properly reset rule selection and loading state when API calls fail
  • Improve copy and message formatting

@xcrzx xcrzx added release_note:skip Skip the PR/issue when compiling release notes Feature:Detection Rules Security Solution rules and Detection Engine Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team v8.9.0 labels Jun 15, 2023
@xcrzx xcrzx self-assigned this Jun 15, 2023
@xcrzx xcrzx requested a review from a team as a code owner June 15, 2023 15:52
@xcrzx xcrzx requested a review from spong June 15, 2023 15:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@xcrzx xcrzx requested review from jpdjere and removed request for spong June 15, 2023 15:53
}
return toastMessage;
return toastMessages.join(' ');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Little curious about the toast UX here since skipped + failed are included in the onSuccess toast (so long as the explicit onError hasn't occurred.)

Splitting out into separate toasts feels like toast spam, and doesn't look like we can add \n's to toast titles, so it ends up looking something like this:

image

Maybe a toast w/ title + description would work here if toastMessages.length > 1, and then have each message on a new line in the description? Could also color a warning toast if skipped or failed were > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a toast w/ title + description would work here if toastMessages.length > 1, and then have each message on a new line in the description? Could also color a warning toast if skipped or failed were > 0?

I like the suggestion. Thank you, @spong 👍 I'm going to implement it in a follow-up

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM! 👍 Left one nit/thought on how we compose the resulting toast, but no changes necessary. Thanks @xcrzx!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #2 / Alert details expandable flyout right panel overview tab insights section should display correlations section
  • [job] [logs] Investigations - Security Solution Tests #3 / Alert details expandable flyout right panel overview tab insights section should display correlations section

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 10.8MB 10.8MB -330.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 13 15 +2
securitySolution 410 414 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 14 16 +2
securitySolution 493 497 +4
total +6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @xcrzx

Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these fixes! 👍

Hope it's fine by you, I'm getting this merged to prevent conflicts with the ML Jobs popover work I'm doing

@jpdjere jpdjere merged commit a8ef2c8 into elastic:main Jun 16, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Detection Rules Security Solution rules and Detection Engine release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants