Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security Solutions][Endpoint] Running-processes rename and refactor to processes #135569

Merged
merged 10 commits into from
Jul 5, 2022
Merged

[Security Solutions][Endpoint] Running-processes rename and refactor to processes #135569

merged 10 commits into from
Jul 5, 2022

Conversation

dasansol92
Copy link
Contributor

Summary

  • Renames running-processes command to processes.
  • Refactor all types, files and i18n keys.
  • Handle api errors when creating processes request.
  • Add unit test for error handling.

For maintainers

@dasansol92 dasansol92 added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0 labels Jun 30, 2022
paul-tavares
paul-tavares reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look through and it looks good me

kevinlog
kevinlog approved these changes Jun 30, 2022
View reviewed changes
Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some tests to fix, but LGTM - feel free to merge on Monday when you resolve it

ashokaditya
ashokaditya reviewed Jul 4, 2022
View reviewed changes
Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great. I just have a couple of questions.

x-pack/plugins/security_solution/common/endpoint/data_generators/endpoint_action_generator.ts
@@ -19,7 +19,7 @@ import {
LogsEndpointAction,
LogsEndpointActionResponse,
RESPONSE_ACTION_COMMANDS,
RunningProcessesEntry,
ProcessesEntry,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably missed this earlier. Please import these as type. I'm going to try and find where to enforce this setting. Some places eslint enforces and in other places it doesn't.

...ublic/management/components/endpoint_responder/endpoint_response_actions_console_commands.ts Outdated
@@ -67,14 +67,14 @@ export const getEndpointResponseActionsConsoleCommands = (
},
},
{
name: 'running-processes',
name: 'processes',
about: i18n.translate(
'xpack.securitySolution.endpointConsoleCommands.runninProcesses.about',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps change the translation key as well to ...Commands.processes.about

x-pack/plugins/security_solution/public/management/mocks/response_actions_http_mocks.ts Outdated
@@ -109,21 +107,21 @@ export const responseActionsHttpMocks = httpHandlerMockFactory<ResponseActionsHt
},
},
{
id: 'runningProcesses',
id: 'processes',
path: GET_RUNNING_PROCESSES_ROUTE,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the route name and API path as well for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the const name. I don't think we want to change the API route.

...s/security_solution/public/management/components/endpoint_responder/get_processes_action.tsx Outdated
const actionRequestSent = Boolean(store.actionRequestSent);

const getRunningProcessesApi = useSendGetEndpointRunningProcessesRequest();
const getProcessesApi = useSendGetEndpointProcessesRequest();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't want to destructure this object to {mutate, data, ...} etc.

@dasansol92 dasansol92 requested a review from ashokaditya July 4, 2022 13:49
@kevinlog kevinlog mentioned this pull request Jul 4, 2022
Merged
2 tasks
ashokaditya
ashokaditya approved these changes Jul 4, 2022
View reviewed changes
...ublic/management/components/endpoint_responder/endpoint_response_actions_console_commands.ts
@@ -83,7 +80,7 @@ export const getEndpointResponseActionsConsoleCommands = (
required: false,
allowMultiples: false,
about: i18n.translate(
'xpack.securitySolution.endpointConsoleCommands.isolate.arg.comment',
'xpack.securitySolution.endpointConsoleCommands.processes.arg.comment',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@dasansol92 dasansol92 marked this pull request as ready for review July 5, 2022 06:39
@dasansol92 dasansol92 requested review from a team as code owners July 5, 2022 06:39
@dasansol92 dasansol92 requested a review from joeypoon July 5, 2022 06:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@dasansol92
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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 5.2MB 5.2MB +468.0B

History

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

@dasansol92 dasansol92 requested a review from ashokaditya July 5, 2022 09:43
@dasansol92 dasansol92 merged commit f94d5ff into elastic:main Jul 5, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-tavares paul-tavares paul-tavares left review comments

@ashokaditya ashokaditya ashokaditya approved these changes

@kevinlog kevinlog kevinlog approved these changes

@joeypoon joeypoon Awaiting requested review from joeypoon joeypoon was automatically assigned from elastic/security-onboarding-and-lifecycle-mgt

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dasansol92 @elasticmachine @kibana-ci @ashokaditya @kevinlog @paul-tavares @kibanamachine