-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] DetectionRulesClient
: return RuleResponse
from createCustomRule
and createPrebuiltRule
#185748
[Security Solution] DetectionRulesClient
: return RuleResponse
from createCustomRule
and createPrebuiltRule
#185748
Conversation
DetectionRulesClient
: return RuleResponse
from public methodsDetectionRulesClient
: return RuleResponse
from createCustomRule
and createPrebuiltRule
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
/ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the changes but held off testing the PR.
Let's revisit the solution with DetectionRulesClientValidationError
and try to find a cleaner one. Also, there's inconsistency in error handling between the createCustomRule
and createPrebuiltRule
methods.
...ity_solution/server/lib/detection_engine/rule_management/api/rules/create_rule/route.test.ts
Show resolved
Hide resolved
...tion_engine/rule_management/logic/detection_rules_client/detection_rules_client_interface.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_management/logic/detection_rules_client/methods/create_custom_rule.ts
Outdated
Show resolved
Hide resolved
...etection_engine/rule_management/logic/detection_rules_client/methods/create_prebuilt_rule.ts
Outdated
Show resolved
Hide resolved
...y_solution/server/lib/detection_engine/rule_management/logic/detection_rules_client/utils.ts
Outdated
Show resolved
Hide resolved
Thanks for the review folks! Moving it to Draft now while I do the changes. Will notify you guys once it's ready for another look. |
Addressed the feedback. Reopening the PR for review. |
/ci |
/ci |
cc2382c
to
74ff7a0
Compare
/ci |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Unknown metric groupsESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @nikitaindik |
…m `createCustomRule` and `createPrebuiltRule` (elastic#185748) **Partially addresses: elastic#184364 ## Summary This PR changes `createCustomRule` and `createPrebuiltRule` methods to return `RuleResponse` instead of `RuleAlertType`. This is a continuation of the effort to improve `DetectionRulesClient`. As a part of it we want to make the client return `RuleResponse` from all its public methods. This is good because it lets us hide rule's internal structure and centralise conversions in one place – in the client. So in this and upcoming PRs we are going to convert `RuleAlertType` to `RuleResponse` within the client's methods and return the `RuleResponse` object.
…m all methods (#186179) **Partially addresses: #184364 ## Summary This PR is a follow-up to [PR #185748](#185748) and it converts the remaining `DetectionRulesClient` methods to return `RuleResponse`. Changes in this PR: - These methods now return `RuleResponse` instead of internal `RuleAlertType` type: - `updateRule` - `patchRule` - `upgradePrebuiltRule` - `importRule`
…m all methods (elastic#186179) **Partially addresses: elastic#184364 ## Summary This PR is a follow-up to [PR elastic#185748](elastic#185748) and it converts the remaining `DetectionRulesClient` methods to return `RuleResponse`. Changes in this PR: - These methods now return `RuleResponse` instead of internal `RuleAlertType` type: - `updateRule` - `patchRule` - `upgradePrebuiltRule` - `importRule`
Partially addresses: #184364
Summary
This PR changes
createCustomRule
andcreatePrebuiltRule
methods to returnRuleResponse
instead ofRuleAlertType
.This is a continuation of the effort to improve
DetectionRulesClient
. As a part of it we want to make the client returnRuleResponse
from all its public methods. This is good because it lets us hide rule's internal structure and centralise conversions in one place – in the client. So in this and upcoming PRs we are going to convertRuleAlertType
toRuleResponse
within the client's methods and return theRuleResponse
object.