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][Bulk actions]- Fix bulk actions data views behavior #138448

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Aug 9, 2022

Summary

Addresses bug found where even when overwrite_data_views was false, the rule's index property was being modified.

Please see added integration tests to understand desired behavior of changes. There is one edge case which is a bit weird, but I think too late to address in 8.4. If a user uses bulk delete on a rule with a data view and no index patterns defined and overwrite_data_views = true, both data view and index will be set to undefined. Per our current behavior, a rule with no data source defaults to using the default index patterns. Not sure this is ideal, but it is in line with the behavior that already exists for a rule.

Checklist

For maintainers

@yctercero yctercero added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Solution Platform Security Solution Platform Team v8.4.0 v8.5.0 labels Aug 9, 2022
@yctercero yctercero self-assigned this Aug 9, 2022
@yctercero yctercero requested review from a team as code owners August 9, 2022 18:46
@yctercero yctercero requested a review from banderror August 9, 2022 18:46
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

cc @yctercero

@yctercero yctercero enabled auto-merge (squash) August 9, 2022 21:01
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Looks fantastic. Thanks for the tests and the quick turnaround here.

expect(editedRuleParams).toHaveProperty('index', ['test-*']);
});

test('should set dataViewId to undefined and index to undefined if overwrite_data_views=true on delete_index_patterns action and rule had no index patterns to begin with', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -59,6 +71,10 @@ const applyBulkActionEditToRuleParams = (
"Index patterns can't be overwritten. Machine learning rule doesn't have index patterns property"
);

if (ruleParams.dataViewId != null && !action.overwrite_data_views) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe encapsulate this in a declarative helper? e.g.

Suggested change
if (ruleParams.dataViewId != null && !action.overwrite_data_views) {
if (actionNotValid(action, ruleParams)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽 I think there's a different helper method @banderror had suggested we could use. Can definitely cleanup/revisit post 8.4.

@yctercero yctercero merged commit 9e8b5b9 into elastic:main Aug 9, 2022
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 9, 2022
…or (elastic#138448)

## Summary

Addresses [bug](elastic#138383) found where even when `overwrite_data_views` was false, the rule's `index` property was being modified.

Please see added integration tests to understand desired behavior of changes. There is one edge case which is a bit weird, but I think too late to address in 8.4. If a user uses bulk delete on a rule with a data view and _no_ index patterns defined and `overwrite_data_views = true`, both data view and index will be set to `undefined`. Per our current behavior, a rule with no data source defaults to using the default index patterns. Not sure this is ideal, but it is in line with the behavior that already exists for a rule.

(cherry picked from commit 9e8b5b9)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 10, 2022
…or (elastic#138448) (elastic#138466)

## Summary

Addresses [bug](elastic#138383) found where even when `overwrite_data_views` was false, the rule's `index` property was being modified.

Please see added integration tests to understand desired behavior of changes. There is one edge case which is a bit weird, but I think too late to address in 8.4. If a user uses bulk delete on a rule with a data view and _no_ index patterns defined and `overwrite_data_views = true`, both data view and index will be set to `undefined`. Per our current behavior, a rule with no data source defaults to using the default index patterns. Not sure this is ideal, but it is in line with the behavior that already exists for a rule.

(cherry picked from commit 9e8b5b9)

Co-authored-by: Yara Tercero <[email protected]>
@yctercero yctercero deleted the update_bulk_actions_dv_logic branch August 10, 2022 02:08
vitaliidm added a commit that referenced this pull request Sep 28, 2022
#141915)

## Summary

- addresses #135201 
- adds Data View cypress and integration tests according to [Data view Bulk Edit test plan](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#heading=h.j583i3o7bg2g) (internal document)
- integration tests were added earlier in #138448
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 28, 2022
elastic#141915)

## Summary

- addresses elastic#135201
- adds Data View cypress and integration tests according to [Data view Bulk Edit test plan](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#heading=h.j583i3o7bg2g) (internal document)
- integration tests were added earlier in elastic#138448

(cherry picked from commit c7301e5)
kibanamachine added a commit that referenced this pull request Sep 29, 2022
#141915) (#142090)

## Summary

- addresses #135201
- adds Data View cypress and integration tests according to [Data view Bulk Edit test plan](https://docs.google.com/document/d/116x7ITTTJQ6cTiwaGK831_f6Ox7XB3qyLiHxC3Cmf8w/edit#heading=h.j583i3o7bg2g) (internal document)
- integration tests were added earlier in #138448

(cherry picked from commit c7301e5)

Co-authored-by: Vitalii Dmyterko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Security Solution Platform Security Solution Platform Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants