Skip to content

Add UUID to RuleAction#148038

Merged
ersin-erdal merged 86 commits intoelastic:mainfrom
ersin-erdal:action-uuid
Feb 22, 2023
Merged

Add UUID to RuleAction#148038
ersin-erdal merged 86 commits intoelastic:mainfrom
ersin-erdal:action-uuid

Conversation

@ersin-erdal
Copy link
Copy Markdown
Contributor

@ersin-erdal ersin-erdal commented Dec 23, 2022

Resolves: #149587

This PR intends to add a uuid field to the rule actions.

A migration script add a uuid to all the existing actions.

Create method of the rule_client (and create endpoint) accepts a rule.actions data without uuid and adds a new uuid to the all actions.

Update and bulkEdit methods of the rule_client (and update and bulk_edit endpoints) accepts rule.actions data with and without uuid. As a user can add a new action to an existing rule, UI should send the uuid of an existing action back then update and bulkEdit methods would add the uuid's to all the new actions.

All the get methods return uuid in the actions.

Since we don't query by the uuid of an action, I marked actions as dynamic: false in the mappings so we don't need to add the uuid to the mappings.

I tried not to modify the legacy APIs, therefore i used some type castings there, but please let me know if they need to be modified as well.

To verify:

Create a rule with some actions and save.
Then add some more actions and expect all of them to have an auto-generated uuid field and still work as they were.

Note:

Initialy uuid in RuleAction was not optional but as it created tons of changes on types and tests we decided to add it optional. It is still required in RawRuleAction which reflects the action object in Kibana SO.
There are 3 APIs in the rulesClient that adds a uuid: create, update, bulkEdit. All of them generates a uuid if there isn't any in an action, and since there is amigration script to add uuid to the existing actions, all the actions would have a uuid...

@ersin-erdal ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.7.0 labels Dec 23, 2022
@ersin-erdal ersin-erdal force-pushed the action-uuid branch 7 times, most recently from 8536609 to 56e33a9 Compare December 24, 2022 20:54
make uuid type generic
@ersin-erdal ersin-erdal force-pushed the action-uuid branch 15 times, most recently from 75df7ea to 8bbfa52 Compare December 29, 2022 00:52
Copy link
Copy Markdown
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

Security solution changes generally LGTM now, thanks for making the updates! I noticed a few last type casts that look like they're leftovers that can be removed now.

},
},
],
] as unknown as RuleAction[],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This type cast and the 2 below seem unnecessary now that uuid is optional?

Copy link
Copy Markdown
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

@ersin-erdal Fantastic! This is so much simpler than the previous implementations. Just one nit comment for your consideration, otherwise LGTM 👍

actionRef: '1',
actionTypeId: '1',
params: { foo: true },
uuid: expect.any(String),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ersin-erdal Sorry, what I meant is checking that the uuid must not be an empty string. I believe the assertion should throw if uuid: '', but expect.any(String) would not throw in this case. Anyway, that's a rather nitpicky comment.

@ersin-erdal
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

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
@kbn/securitysolution-io-ts-alerting-types 119 121 +2
alerting 475 476 +1
triggersActionsUi 531 532 +1
total +4

Async chunks

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

id before after diff
securitySolution 13.7MB 13.7MB +483.0B
triggersActionsUi 919.6KB 919.7KB +44.0B
total +527.0B

Page load bundle

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

id before after diff
triggersActionsUi 80.4KB 80.4KB +22.0B
Unknown metric groups

API count

id before after diff
@kbn/securitysolution-io-ts-alerting-types 138 140 +2
alerting 486 487 +1
triggersActionsUi 560 561 +1
total +4

ESLint disabled line counts

id before after diff
securitySolution 427 426 -1

Total ESLint disabled count

id before after diff
securitySolution 503 502 -1

History

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

cc @ersin-erdal

Copy link
Copy Markdown
Contributor

@WafaaNasr WafaaNasr left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM!
Great work, Thanks @ersin-erdal for clarifying why the uuid is optional in some places :) in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:feature Makes this part of the condensed release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add uuid to rule actions