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.

Sign up for GitHub

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

Jump to bottom

[RAM][SECURITYSOLUTION][ALERTS] - Update action form UI to fit security solution needs (#154531) #154526

Merged
merged 16 commits into from Apr 13, 2023
Merged

[RAM][SECURITYSOLUTION][ALERTS] - Update action form UI to fit security solution needs (#154531) #154526

merged 16 commits into from Apr 13, 2023

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Apr 6, 2023 •
edited

Summary

These changes customise action form UI for security solution needs.

image

To see how new design works in security solution, you can use this draft PR which consists of all the changes. I split that big PR into smaller independent pieces for easier review process.

Fixes #154531

@e40pud e40pud self-assigned this Apr 6, 2023
@e40pud e40pud changed the title [Security Solution][Alerts] - Customized action form [RAM][SECURITYSOLUTION][ALERTS] - Update action form UI to fit security solution needs (#154531) Apr 6, 2023
@e40pud
Copy link
Contributor Author

e40pud commented Apr 6, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 6, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 8, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 8, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 9, 2023

@elasticmachine merge upstream

@e40pud e40pud marked this pull request as ready for review April 9, 2023 18:44
@e40pud e40pud requested review from a team as code owners April 9, 2023 18:44
Zacqary
Zacqary reviewed Apr 10, 2023
View reviewed changes
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

I'm unsure if these issues are blockers for this PR, or something that we should open a tech debt issue about. I'm open to more input about that.

In summary, I'm concerned about adding feature-specific code to a generic platform plugin, instead of adding a pathway to customize it with props.

@e40pud
Copy link
Contributor Author

e40pud commented Apr 10, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 10, 2023

@Zacqary Thanks for the review! I agree that the approach is not ideal. We discussed this with @XavierM couple weeks ago and decided that it will be the easiest and probably enough as a solution for the task that we are doing.

I will revisit these changes and will see how to improve them by passing security solution specific parameters from the outside.

@e40pud
Copy link
Contributor Author

e40pud commented Apr 11, 2023

@elasticmachine merge upstream

@e40pud e40pud requested a review from Zacqary April 11, 2023 11:55
@e40pud
Copy link
Contributor Author

e40pud commented Apr 11, 2023

@elasticmachine merge upstream

Zacqary
Zacqary approved these changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

LGTM!

@e40pud
Copy link
Contributor Author

e40pud commented Apr 12, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Apr 12, 2023

@elasticmachine merge upstream

jpdjere
jpdjere reviewed Apr 12, 2023
View reviewed changes
x-pack/plugins/alerting/common/rule.ts
@@ -77,6 +77,12 @@ export interface RuleExecutionStatus {
export type RuleActionParams = SavedObjectAttributes;
export type RuleActionParam = SavedObjectAttribute;

export interface RuleActionFrequency extends SavedObjectAttributes {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this interface need to extend from SavedObjectAttributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do rule conversion into a RawRule which is an extension of SavedObjectAttributes here.

From my understanding server side uses SavedObjectAttributes as an interface for saved objects and objects that we pass to the server needs to be compatible with that interface.

@Zacqary correct me if I'm wrong or if there is anything else you can add here.

jpdjere
jpdjere approved these changes Apr 12, 2023
View reviewed changes
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.

@e40pud

Detections Rules Codeowners files LGTM 👍

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 546 550 +4
triggersActionsUi 507 511 +4
total +8

Async chunks

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

id before after diff
securitySolution 15.9MB 15.9MB +5.0KB
triggersActionsUi 1.4MB 1.4MB +857.0B
total +5.8KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 58.1KB 58.2KB +138.0B
Unknown metric groups

API count

id before after diff
alerting 567 571 +4
triggersActionsUi 536 540 +4
total +8

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

References to deprecated APIs

id before after diff
alerting 74 76 +2

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

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

cc @e40pud

@e40pud e40pud merged commit 124b919 into elastic:main Apr 13, 2023
20 checks passed
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 13, 2023
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Apr 19, 2023
@e40pud @kibanamachine @saarikabhasi
[RAM][SECURITYSOLUTION][ALERTS] - Update action form UI to fit securi…
aeaa370 
…ty solution needs (elastic#154531) (elastic#154526)

## Summary

These changes customise action form UI for security solution needs.


![image](https://user-images.githubusercontent.com/2700761/230332242-20cbd4a1-8f76-4e1d-835e-cd0525cc530c.png)

To see how new design works in security solution, you can use this
[draft PR](elastic#153113) which consists
of all the changes. I split that big PR into smaller independent pieces
for easier review process.

Fixes elastic#154531

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@KOTungseth KOTungseth added the Team:Detection Alerts Security Detection Alerts Area Team label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zacqary Zacqary Zacqary approved these changes

@jpdjere jpdjere jpdjere approved these changes

@XavierM XavierM Awaiting requested review from XavierM

Assignees

@e40pud e40pud

Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Detection Alerts Security Detection Alerts Area Team v8.8.0
Projects
No open projects
AppEx: ResponseOps - Rules & Alerts Management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RAM][SECURITYSOLUTION][ALERTS] - Update action form UI to fit security solution needs
6 participants
@e40pud @kibana-ci @Zacqary @jpdjere @KOTungseth @kibanamachine