-
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
EMT-339: policy response schema, views and tests #65962
EMT-339: policy response schema, views and tests #65962
Conversation
@elasticmachine merge upstream |
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.
Some initial feedback
*/ | ||
export interface HostPolicyResponseActionDetails { | ||
export interface HostPolicyResponseAppliedAction { | ||
name: 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.
It would be nice if we could have a permissive list of possible actions that we could use here and everywhere else where we do string[]
. Some like this:
type HostPolicyActionName =
| 'download_model'
| 'ingest_events_config'
| 'workflow'
| 'configure_elasticsearch_connection'
| 'configure_kernel'
| 'configure_logging'
| 'configure_malware'
| 'connect_kernel'
| 'detect_file_open_events'
| 'detect_file_write_events'
| 'detect_image_load_events'
| 'detect_process_events'
| 'download_global_artifacts'
| 'load_config'
| 'load_malware_model'
| 'read_elasticsearch_config'
| 'read_events_config'
| 'read_kernel_config'
| 'read_logging_config'
| 'read_malware_config'
| string;
Then on this line (and other areas below):
name: string; | |
name: HostPolicyActionName; |
Tried it quickly on my IDE and seems to work ok even with custom unknown strings:
export type HostPolicyResponseConfiguration = HostPolicyResponse['endpoint']['policy']['applied']['response']['configurations']; | ||
|
||
interface HostPolicyResponseConfigurationStatus { | ||
status: HostPolicyResponseActionStatus; | ||
concerned_actions: Array<keyof HostPolicyResponseActions>; | ||
concerned_actions: 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.
If you implement the suggestion above, this would then be:
concerned_actions: string[]; | |
concerned_actions: HostPolicyActionName[] |
@@ -80,16 +80,18 @@ export const hostMiddlewareFactory: ImmutableMiddlewareFactory<HostState> = core | |||
version: '1.0.0', | |||
status: HostPolicyResponseActionStatus.success, | |||
id: '17d4b81d-9940-4b64-9de5-3e03ef1fb5cf', | |||
actions: { | |||
download_model: { | |||
actions: [ |
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.
you probably added this only because @parkiino has not yet merged her change that integrates with API, correct (and to suppress ts errors)?
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.
Yes just to get the test to pass.
@@ -81,7 +83,7 @@ export const policyResponseFailedOrWarningActionCount: ( | |||
*/ | |||
export const policyResponseActions: ( | |||
state: Immutable<HostState> | |||
) => undefined | Partial<HostPolicyResponseActions> = createSelector( | |||
) => undefined | ImmutableArray<HostPolicyResponseAppliedAction> = createSelector( |
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.
Just use Immutable<>
instead of ImmutableArray<>
. That type will take care of using the appropriate Immutable type in its implementation. I saw Rob provide this feedback recently when I saw him providing the same feedback - the reason being: if Typescript introduces a builtin Immutable
Generic, then we will be in a better place to remove our implementation.
}: { | ||
actions: Immutable<Array<keyof HostPolicyResponseActions>>; | ||
actionStatus: Partial<HostPolicyResponseActions>; | ||
actions: ImmutableArray<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.
Use Immutable<>
(same below in several places). Also, would this change based on my prior suggestion to type up known action names?
…e/kibana into EMT-339_update_schema_views
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Closed for #66264 |
Summary
Checklist