[DE] - Investigation fields followup #164133

yctercero · 2023-08-17T05:29:57Z

Summary

Please ignore the first 42 commits - they got pulled in from my original PR since I branched off it before it'd been merged to main. Follow up PR to #163235 - fixing types and adding tests.

There had been discussion about whether the new investigation_fields should be something like:

t.array(NonEmptyString)

Or, if it should be an object like:

t.intersection([
  t.exact(
    t.type({
      fields: NonEmptyArray(t.string),
    })
  ),
  t.exact(
    t.partial({
      overide: t.boolean,
    })
  ),
]);

The argument for making it an object is that when expanding features related to this, we can simply add keys to the object instead of needing to add another top level field. Georgii had also pointed out that it would make the schema more compatible with the DiffableRule interface and the rule diff/upgrade algorithm. Seeing as there is already discussion on how this can be expanded in the future, it seemed appropriate to move forward with updating this field type to be an object.

The second point that came up during initial PR review was whether this field should be made defaultable. This would mean including migration scripts and having the investigation_fields always be present in the schema. I feel that if we kept the type of this new field as string[] I would not feel strongly one way or the other about making this a default field. However, in following the same pattern as alert suppression, I feel that the lack of this field in the schema should represent the fact that this value is unset and not in use. I also feel that carrying around an object, however small, when not in use for a completely optional field does not make sense at this time.

By making investigation_fields.fields be a NonEmptyArray(t.string), we know that if the object is present on the schema, fields are specified and in use. We won't ever end up with something where the fields array is empty, but the other possible future configuration params are still set. I think that makes it clear and easy to work with in the UI and as an API user.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ted_fields

…-ref HEAD~1..HEAD --fix'

…ted_fields

…na into custom_highlighted_fields

…-ref HEAD~1..HEAD --fix'

…-fix'

kibanamachine · 2023-08-29T04:44:07Z

expected head sha didn’t match current head ref.

yctercero · 2023-08-29T04:57:00Z

@elasticmachine merge upstream

…kibana into investigation_fields_followup

yctercero · 2023-08-29T07:03:44Z

@elasticmachine merge upstream

yctercero · 2023-08-29T07:04:18Z

@elasticmachine merge upstream

kibana-ci · 2023-08-29T08:11:04Z

💚 Build Succeeded

Buildkite Build
Commit: 10f6317

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	12.6MB	12.6MB	+817.0B

History

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

cc @yctercero

kibanamachine · 2023-08-29T12:46:54Z

💔 All backports failed

Status	Branch	Result
❌	8.10	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 164133