-
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][Detections] Extend alerts schema to accommodate the list of assigned users (#7647) #166845
[Security Solution][Detections] Extend alerts schema to accommodate the list of assigned users (#7647) #166845
Conversation
…he list of assigned users (elastic#7647)
Pinging @elastic/security-solution (Team: SecuritySolution) |
export type { Ancestor890 as Ancestor8110 }; | ||
|
||
export interface BaseFields8110 extends BaseFields890 { | ||
[ALERT_WORKFLOW_ASSIGNEE_IDS]: string[]; |
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.
as ALERT_WORKFLOW_ASSIGNEE_IDS defined as not required
[ALERT_WORKFLOW_ASSIGNEE_IDS]: {
type: 'keyword',
array: true,
required: false,
},
should it be added here as optional property?
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.
good question, I mimicked the same behaviour we have for workflow_tags
that we introduced in 8.9 https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/api/detection_engine/model/alerts/8.9.0/index.ts#L29. It is also not required.
@dplumlee do you remember why we skipped making tags optional property?
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.
Made this field optional, @dplumlee let me know if there is a reason to keep it required here.
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 don't recall if there was a specific reason for not making the tags optional here, I believe since it was a new field, our use case just wanted to always count on it being there. I think the right way to go is keeping it aligned with the field maps definition as @vitaliidm points out though, I probably shouldn't have listed ours as optional in that file.
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.
LGTM from explore side, thanks!
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.
it looks good to me
Left comment on optional typing: with undefined
it still will be required to pass everywhere, so proposing to use question mark
just one, note, once you make ALERT_WORKFLOW_ASSIGNEE_IDS
I don't think you need to add empty array everywhere
x-pack/plugins/security_solution/common/api/detection_engine/model/alerts/8.11.0/index.ts
Show resolved
Hide resolved
…eature/alert-user-assignment-7647
💔 Build FailedFailed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: cc @e40pud |
23f64ed
into
elastic:security/feature/alert-user-assignment
Summary
Closes https://github.com/elastic/security-team/issues/7647
This PR extends alert's schema. We add a new field
kibana.alert.workflow_assignee_ids
where assignees will live.