-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Detection Engine] changes codeowners for rule create/edit form components #173906
[Security Solution][Detection Engine] changes codeowners for rule create/edit form components #173906
Conversation
/ci |
/ci |
/ci |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
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.
Code changes LGTM for Explore team!
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.
Thanks so much for getting to this - I think it's super helpful. I don't want to block as this touches a ton of files and I think it's a good move forward. I am wondering if we could reduce a bit of the complexity by merging some of these folders so the structure might be something like:
detections_response
--> rule_creation
--> components
--> common
--> containers
--> logic
--> pages (what you have as rule_creation_ui right now)
--> rule_details
--> components
--> common
--> containers
--> logic
--> pages (what you have as rule_details_ui right now)
--> rule_edit
--> components
--> common
--> containers
--> logic
--> pages
@yctercero , thanks for feedback and proposal Changes in PR are based on adopted earlier folder structure(#142950, a lot more in description with diagrams) and follow existing approach. |
Makes sense! I don't think further restructure is a priority and like you mentioned the goal is to update ownership and this does just that! Thanks, I'll LGTM. |
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.
Rule Management changes LGTM 👍
Components moved and components left in public/detections/components/rules
make sense. I left a couple of minor questions.
Great refactoring, thank you @vitaliidm!
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.
I think MITRE components and model should be either under public/detection_engine/rule_management
or its own generic subdomain public/detection_engine/mitre
. Maybe except for the code that implements the form inputs for MITRE mappings, not sure.
Updating MITRE mappings is on the Rule Management team, specifically, @dplumlee used to do that work. We also own the MITRE Coverage Overview page.
I'd be ok merging this as is and taking a closer look at the public/detection_engine/rule_creation_ui/components/mitre
folder later.
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.
Nit: Just curious why components for the About, Define, and Schedule sections are under rule_creation_ui
, but the one for the Actions section is under rule_creation
?
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.
actions component is used within details section: https://github.com/elastic/kibana/pull/173906/files#diff-910140c9d6034d2b98eab04e82f22ac18451fe485e1951997ad22004b8920f56R66
I also notices, Details component used there too, so moved it as well to rule_creation
. The rest are still in rule_creation_ui
@yctercero @vitaliidm regarding #173906 (review): The complexity is objectively there, but it has a reason. Our pages are complex and contain pieces from different subdomains. For instance, the Rule Details page contains: the rule details functionality itself, rule management actions, some ML jobs UI, rule monitoring UI, alerts table, exceptions. We can't keep all that code under a single That's why we need at least two layers of code on the FE side: a layer of subdomains and a layer for pages. In #142950 we proposed that pages should live in "UI subdomains". Maybe that's what confuses people and we just need to tweak the folder structure a bit to remove this confusion. Something like that:
++ for "If we would like to change it, I propose to start discussion outside of this PR and agree on a new folder structure" |
Thanks for the background on it. I do like that suggested structure. Though I think it's not a priority right now. Maybe if we get feedback from devs that it's confusing we can re-examine. |
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.
Defend Workflows changes LGTM 👍
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.
THI changes lgtm 👍
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
…ate/edit form components (elastic#173906) ## Summary - addresses elastic#145420 - moves almost all of the components from `/x-pack/plugins/security_solution/public/detections/components/rules` to - `x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components` , **rule_creation_ui** components that used only in rule creation are placed here - `x-pack/plugins/security_solution/public/detection_engine/rule_creation/components` , **rule_creation** components that used also in another areas(details, or rule tables), placed here - some of the basic components, like `technical_preview_badge` or `severity_badge` moved to common area: `x-pack/plugins/security_solution/public/common/components` The rest of the components, used by rule-management team only, left `/x-pack/plugins/security_solution/public/detections/components/rules`. **rule_creation** and **rule_creation_ui** folders owned by detection-engine team ## Next steps? Since the main purpose of this PR to change codeownership of components, there are still quite a few places where different types are imported from `rule_creation_ui` folder. [For example](https://github.com/elastic/kibana/pull/173906/files#diff-4162e1e577d6b1891a6865ffea6950a9e8a04183e4ea345659bd04fd31ed135dR23). Type `FieldValueQueryBar` is imported in timeline component. With this refactoring, it becomes much easier to identify such imports and refactor it further in future --------- Co-authored-by: kibanamachine <[email protected]>
Summary
/x-pack/plugins/security_solution/public/detections/components/rules
tox-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/components
, rule_creation_uicomponents that used only in rule creation are placed here
x-pack/plugins/security_solution/public/detection_engine/rule_creation/components
, rule_creationcomponents that used also in another areas(details, or rule tables), placed here
technical_preview_badge
orseverity_badge
moved to common area:x-pack/plugins/security_solution/public/common/components
The rest of the components, used by rule-management team only, left
/x-pack/plugins/security_solution/public/detections/components/rules
.rule_creation and rule_creation_ui folders owned by detection-engine team
Next steps?
Since the main purpose of this PR to change codeownership of components, there are still quite a few places where different types are imported from
rule_creation_ui
folder. For example. TypeFieldValueQueryBar
is imported in timeline component. With this refactoring, it becomes much easier to identify such imports and refactor it further in future