Skip to content

[Security Solution][Defend Workflows] Fix type errors indicated by TS 4.7.4.#163066

Merged
gergoabraham merged 13 commits intoelastic:mainfrom
gergoabraham:chore/defend-workflows-fix-ts-4.7.4-type-errors
Sep 18, 2023
Merged

[Security Solution][Defend Workflows] Fix type errors indicated by TS 4.7.4.#163066
gergoabraham merged 13 commits intoelastic:mainfrom
gergoabraham:chore/defend-workflows-fix-ts-4.7.4-type-errors

Conversation

@gergoabraham
Copy link
Copy Markdown
Contributor

@gergoabraham gergoabraham commented Aug 3, 2023

Summary

TypeScript 4.7.4 shows some errors in our codebase, and the errors are suppressed in PR #162738. This will be merged after the linked PR.

This PR modifies the code so there's no need for suppressing the errors. There were 2 lint errors I couldn't fix.


1️⃣
The following one I didn't manage to fix without a deeper refactor (link)

// x-pack/plugins/security_solution/public/common/mock/endpoint/app_context_render.tsx

  const store = createStore(
    mockGlobalState,
    storeReducer,
    kibanaObservable,
    storage,
    // @ts-expect-error ts upgrade v4.7.4
    [...managementMiddlewareFactory(coreStart, depsStart), middlewareSpy.actionSpyMiddleware]
  );

2️⃣
Also, I have no idea how to fix this (link):

  pipeRun<
    T extends ExtensionPoint['type'],
    D extends NarrowExtensionPointToType<T> = NarrowExtensionPointToType<T>,
    // @ts-expect-error ts upgrade v4.7.4
    P extends Parameters<D['callback']> = Parameters<D['callback']>
  >(
    extensionType: T,
    initialCallbackInput: P[0]['data'],
    callbackContext: ServerExtensionCallbackContext,
    callbackResponseValidator?: (data: P[0]['data']) => Error | undefined
  ): Promise<P[0]['data']>;

Tried by merging D and P, or by creating a helper type using infer to unwrap the type of data, but anyway I got type errors, so after some hours, I just let this go. Please let me know if you got an idea.

@gergoabraham gergoabraham added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Aug 3, 2023
@gergoabraham gergoabraham self-assigned this Aug 3, 2023
@gergoabraham gergoabraham force-pushed the chore/defend-workflows-fix-ts-4.7.4-type-errors branch from 0fa197a to 3ff6048 Compare August 3, 2023 13:42
@gergoabraham gergoabraham marked this pull request as ready for review August 3, 2023 13:49
@gergoabraham gergoabraham requested review from a team as code owners August 3, 2023 13:49
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@gergoabraham gergoabraham force-pushed the chore/defend-workflows-fix-ts-4.7.4-type-errors branch from 3ff6048 to 8a4b0d6 Compare September 1, 2023 12:08
@gergoabraham gergoabraham removed the request for review from a team September 1, 2023 12:15
@gergoabraham gergoabraham marked this pull request as ready for review September 1, 2023 12:15
@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@gergoabraham gergoabraham requested review from ashokaditya and removed request for joeypoon September 4, 2023 07:15
@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

}

// @ts-expect-error ts upgrade v4.7.4
if (isMounted() && OsqueryForm) {
Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl Sep 11, 2023

Choose a reason for hiding this comment

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

@gergoabraham I remember we had some issues here (not typescript but how it works). Are you sure this change preserves the behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am mostly sure, yes...

OsqueryForm comes from useKibana().services.osquery.OsqueryResponseActionTypeForm, which got its value during plugin start, and its value is the return value of getLazyOsqueryResponseActionTypeForm(), which - independently of the lazy load - returns a function component which itself is never falsy, so based on this, I believe there should be no problem.

...but...

I'm happy to test it if you can help what should be tested here. I did some basic tests (i.e. tried to use this component, by throttling network connection so it loads slowly) and there were no issues.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I remember there was some strange behaviour that is why I made this OqueryForm check. I'll test it manually to see if this is not broken now. I am saying this just because there is a strange behavior in the test. It's not succesful - it's pending...
Zrzut ekranu 2023-09-12 o 16 14 11

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tested this manually and I think it still works as expected, thanks!
However - can you please do me a favor and remove the browser filter from here? Lets see if this make the test run then. https://github.com/elastic/kibana/blob/4180a1a105053e5579e6852082bdeaa06f019cd3/x-pack/plugins/osquery/cypress/e2e/all/alerts_response_actions_form.cy.ts#L33C16-L33C16

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thank you for testing it!

removed the browser filters, let's see what happens: dac76a5

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think something is broken, moreover the artifacts are not build. Feel free to revert this commit and just ship your changes. I'll take care of osquery testing in a separate PR. How does this sound?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

revert is done, thanks for the help, @tomsonpl!
cf43db4

@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes and thanks for bearing with me 👍

@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@gergoabraham
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@gergoabraham gergoabraham enabled auto-merge (squash) September 18, 2023 09:35
@gergoabraham gergoabraham merged commit e00aa75 into elastic:main Sep 18, 2023
@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #33 / serverless security UI Case View page "before all" hook for "should show the case view page correctly"
  • [job] [logs] Investigations - Security Solution Cypress Tests #6 / Timeline search and filters Update kqlMode for timeline should be able to update timeline kqlMode with filter should be able to update timeline kqlMode with filter

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 -393.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
osquery 51.1KB 51.1KB +35.0B

History

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

cc @gergoabraham

@gergoabraham gergoabraham deleted the chore/defend-workflows-fix-ts-4.7.4-type-errors branch September 18, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR 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.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants