-
Notifications
You must be signed in to change notification settings - Fork 136
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
Incident Finding event class definition #903
Conversation
Signed-off-by: Rajas <[email protected]>
Signed-off-by: Rajas <[email protected]>
Generally looks good and needed for complettion. Main comment so far: |
Signed-off-by: Rajas <[email protected]>
Signed-off-by: Rajas <[email protected]>
Signed-off-by: Rajas <[email protected]>
Looks good to me as well. The only attribute that I noticed missing was the consolidated attacks array. I suspect it was removed for duplication as it's already a part of each of the findings? |
That's what I would assume as well - the However, there is an argument that the overall Incident could have its own |
Yep, that was the thought, but we can add a top level |
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
Signed-off-by: Rajas <[email protected]>
Signed-off-by: Rajas <[email protected]>
Signed-off-by: Rajas <[email protected]>
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!
Related Issue: #902
Related PR: #786
Description of changes:
A few topics to discuss based on #786 -
findings_info
vsfinding_info_list
- I am okay with either of the two, however finding_info_list makes it easier to distinguish it from finding_info. - based on weekly call -finding_info_list
user
vsassignee
- are both necessary in the class? - keeping assignee, addingassignee_group
incident_uid
- haven't added it,metadata_uid
should be used instead.