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.

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

[Security Solution, Lists] Replace legacy imports from 'elasticsearch' package #107226

Merged
merged 10 commits into from
Aug 5, 2021

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Jul 29, 2021

Summary

This prefers the newer types from '@elastic/elasticsearch'. Relates to #83910.

There was one instance where mock data was insufficient to satisfy the newer analogous types; in all other cases this was just a find/replace.

For maintainers

This prefers the newer types from '@elastic/elasticsearch'.

There was one instance where mock data was insufficient to satisfy the
newer analogous types; in all other cases this was just a find/replace.
@rylnd rylnd added chore v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Jul 29, 2021
@rylnd rylnd self-assigned this Jul 29, 2021
@rylnd rylnd changed the title [Security Solution, Lists]Replace legacy imports from 'elasticsearch' package [Security Solution, Lists] Replace legacy imports from 'elasticsearch' package Jul 29, 2021
@rylnd rylnd marked this pull request as ready for review July 29, 2021 18:58
@rylnd rylnd requested review from a team as code owners July 29, 2021 18:58
@rylnd rylnd requested review from academo and parkiino July 29, 2021 18:58
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

We know that this mock has hits with _source values, but we cannot
convey this to typescript as null assertions are disabled within this
project. This seems like the next best solution, preferable to a
@ts-expect-error.
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM,

What I did:

  • I looked at the types and the commit with the if check for _source and am fine with it.

@mshustov
Copy link
Contributor

I can see a few imports left in x-pack/test/detection_engine_api_integration/* tests. Would you mind migrating them as well?

* refactors destructuring due to _source being properly declared as
  conditional
Changes here fall into one of two categories:

* If the test was making an assertion on a value from _source, we simply
null chain and continue to assert on a possibly undefined value.

* If the test logic depends on _source being present, we first assert that
presence, and exit the test early if absent.
@rylnd
Copy link
Contributor Author

rylnd commented Aug 2, 2021

@FrankHassanabad @mshustov the changes required to fix the integration tests were much more comprehensive than our app code itself; see 93aa2db for details

@rylnd
Copy link
Contributor Author

rylnd commented Aug 3, 2021

@elasticmachine merge upstream

@kibanamachine kibanamachine requested a review from a team as a code owner August 3, 2021 17:12
 Conflicts:
	x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx
Copy link
Member

@pzl pzl left a comment

Choose a reason for hiding this comment

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

Security/OLM 👍

@rylnd rylnd enabled auto-merge (squash) August 5, 2021 17:12
@rylnd
Copy link
Contributor Author

rylnd commented Aug 5, 2021

@elasticmachine merge upstream

@@ -6,7 +6,7 @@
*/

import { useEffect, useRef, useState, useCallback } from 'react';
import { UpdateDocumentByQueryResponse } from 'elasticsearch';
import type { estypes } from '@elastic/elasticsearch';
Copy link
Contributor

Choose a reason for hiding this comment

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

nifty

@@ -94,7 +94,7 @@ export const updateAlertStatusAction = async ({
// TODO: Only delete those that were successfully updated from updatedRules
setEventsDeleted({ eventIds: alertIds, isDeleted: true });

if (response.version_conflicts > 0 && alertIds.length === 1) {
if (response.version_conflicts && alertIds.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this not run if response.version_conflicts === 0 since thats a falsy value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, a value of 0 never satisfied this condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't sure if you wanted it to now given the defaults below (should have added that point). 👍🏾

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see! I think 0 makes sense as the default here, I can't think of a more appropriate one.

);
const fullSignal = fullSource!._source; // If this doesn't exist the test is going to fail anyway so using a bang operator here to get rid of ts error
const fullSignal = fullSource?._source;
if (!fullSignal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏾

@kibanamachine
Copy link
Contributor

💚 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 6.5MB 6.5MB +204.0B

History

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

cc @rylnd

@rylnd rylnd merged commit 8665f36 into elastic:master Aug 5, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 5, 2021
…' package (elastic#107226)

* Remove legacy imports from 'elasticsearch' package

This prefers the newer types from '@elastic/elasticsearch'.

There was one instance where mock data was insufficient to satisfy the
newer analogous types; in all other cases this was just a find/replace.

* Fix type errors with a null guard

We know that this mock has hits with _source values, but we cannot
convey this to typescript as null assertions are disabled within this
project. This seems like the next best solution, preferable to a
@ts-expect-error.

* Fix a few more type errors

* Replace legacy type imports in integration tests

* refactors destructuring due to _source being properly declared as
  conditional

* Update more integration tests to account for our optional _source

Changes here fall into one of two categories:

* If the test was making an assertion on a value from _source, we simply
null chain and continue to assert on a possibly undefined value.

* If the test logic depends on _source being present, we first assert that
presence, and exit the test early if absent.

* Fix more type errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

@rylnd rylnd deleted the replace-legacy-secsol-imports branch August 5, 2021 20:47
kibanamachine added a commit that referenced this pull request Aug 5, 2021
…' package (#107226) (#107808)

* Remove legacy imports from 'elasticsearch' package

This prefers the newer types from '@elastic/elasticsearch'.

There was one instance where mock data was insufficient to satisfy the
newer analogous types; in all other cases this was just a find/replace.

* Fix type errors with a null guard

We know that this mock has hits with _source values, but we cannot
convey this to typescript as null assertions are disabled within this
project. This seems like the next best solution, preferable to a
@ts-expect-error.

* Fix a few more type errors

* Replace legacy type imports in integration tests

* refactors destructuring due to _source being properly declared as
  conditional

* Update more integration tests to account for our optional _source

Changes here fall into one of two categories:

* If the test was making an assertion on a value from _source, we simply
null chain and continue to assert on a possibly undefined value.

* If the test logic depends on _source being present, we first assert that
presence, and exit the test early if absent.

* Fix more type errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Ryland Herrick <ryalnd@gmail.com>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
…' package (elastic#107226)

* Remove legacy imports from 'elasticsearch' package

This prefers the newer types from '@elastic/elasticsearch'.

There was one instance where mock data was insufficient to satisfy the
newer analogous types; in all other cases this was just a find/replace.

* Fix type errors with a null guard

We know that this mock has hits with _source values, but we cannot
convey this to typescript as null assertions are disabled within this
project. This seems like the next best solution, preferable to a
@ts-expect-error.

* Fix a few more type errors

* Replace legacy type imports in integration tests

* refactors destructuring due to _source being properly declared as
  conditional

* Update more integration tests to account for our optional _source

Changes here fall into one of two categories:

* If the test was making an assertion on a value from _source, we simply
null chain and continue to assert on a possibly undefined value.

* If the test logic depends on _source being present, we first assert that
presence, and exit the test early if absent.

* Fix more type errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (392 commits)
  update linting doc (elastic#105748)
  [APM] Various improvements from elastic#104851 (elastic#107726)
  Update dependency @elastic/charts to v33.2.0 (master) (elastic#107842)
  Fix default route link on kibana homepage (elastic#107809)
  [APM] Invalidate trackPageview on route change (elastic#107741)
  Service map backend links (elastic#107317)
  [index patterns] index pattern create modal (elastic#101853)
  [RAC] integrating rbac search strategy with alert table (elastic#107242)
  [Security Solution] Siem signals -> alerts as data field and index aliases (elastic#106049)
  [Metrics UI] Add checkbox to optionally drop partial buckets (elastic#107676)
  [Metrics UI] Fix metric threshold preview regression (elastic#107674)
  Disable Product check in @elastic/elasticsearch-js (elastic#107642)
  [App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling (elastic#107603)
  [Security Solution, Lists] Replace legacy imports from 'elasticsearch' package (elastic#107226)
  [maps] asset tracking tutorial (elastic#104552)
  [scripts/build_ts_refs] when using `--clean` initialize caches (elastic#107777)
  Upgrade EUI to v36.1.0 (elastic#107231)
  [RAC] [TGrid] Implements cell actions in the TGrid (elastic#107771)
  Realign cypress/ccs_integration with cypress/integration (elastic#107743)
  Allow optional OSS to X-Pack dependencies (elastic#107432)
  ...

# Conflicts:
#	x-pack/examples/reporting_example/public/application.tsx
#	x-pack/examples/reporting_example/public/components/app.tsx
#	x-pack/plugins/canvas/public/services/legacy/stubs/reporting.ts
#	x-pack/plugins/reporting/common/types.ts
#	x-pack/plugins/reporting/public/lib/reporting_api_client/context.tsx
#	x-pack/plugins/reporting/public/management/mount_management_section.tsx
#	x-pack/plugins/reporting/public/management/report_listing.test.tsx
#	x-pack/plugins/reporting/public/plugin.ts
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
#	x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants