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][Alerts] Remove dead legacy signals code #128328

Merged
merged 5 commits into from
Mar 29, 2022

Conversation

marshallmain
Copy link
Contributor

Summary

Removes code that was used to generate legacy signals and is no longer used. Updates some unit tests that incorrectly relied on the legacy functions and adds appropriate mocks for PersistenceAlertServices.

@marshallmain marshallmain added backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Alerts Security Detection Alerts Area Team v8.2.0 labels Mar 23, 2022
@marshallmain marshallmain marked this pull request as ready for review March 23, 2022 17:25
@marshallmain marshallmain requested review from a team as code owners March 23, 2022 17:25
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@marshallmain marshallmain added the release_note:skip Skip the PR/issue when compiling release notes label Mar 23, 2022
@@ -134,4 +138,342 @@ describe('buildAlert', () => {
expect(groupId).toEqual(groupIds[0]);
}
});

describe('recursive intersection between objects', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests got lost in the migration - adding them back since "recursively merging objects" is a bit tricky.

@@ -118,3 +117,54 @@ export const buildAlertRoot = (
[ALERT_GROUP_ID]: generateAlertId(doc),
};
};

export const objectArrayIntersection = (objects: object[]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are only being moved from build_bulk_body.ts.

@madirey
Copy link
Contributor

madirey commented Mar 28, 2022

@marshallmain Tried to run this and the alerts table looks broken to me
image

@marshallmain
Copy link
Contributor Author

marshallmain commented Mar 28, 2022

@madirey I've seen that in testing before on main as well sometimes, seems like the default fields don't get loaded properly all the time? Never been able to reproduce reliably. There are no UI changes in this PR though so I'd be surprised if it's related.

It should let you add columns still and display them correctly.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Looks like ResponseOps got pinged for the review for adding the new mock x-pack/plugins/rule_registry/server/utils/create_persistence_rule_type_wrapper.mock.ts.

If that's all it is for ResponseOps, LGTM

isSavedQueryParams(params) ||
isThreatParams(params)
) {
if (!isMachineLearningParams(params)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@madirey madirey 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 doing this, @marshallmain. Code looks good, performed sanity test.

@marshallmain marshallmain merged commit ac4e96c into elastic:main Mar 29, 2022
@marshallmain marshallmain deleted the remove-dead-code branch March 29, 2022 16:00
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 release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants