-
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
[Security Solution, Lists] Replace legacy imports from 'elasticsearch' package #107226
Conversation
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.
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.
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.
LGTM,
What I did:
- I looked at the types and the commit with the if check for
_source
and am fine with it.
I can see a few imports left in |
* 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.
@FrankHassanabad @mshustov the changes required to fix the integration tests were much more comprehensive than our app code itself; see 93aa2db for details |
@elasticmachine merge upstream |
Conflicts: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx
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.
Security/OLM 👍
@elasticmachine merge upstream |
@@ -6,7 +6,7 @@ | |||
*/ | |||
|
|||
import { useEffect, useRef, useState, useCallback } from 'react'; | |||
import { UpdateDocumentByQueryResponse } from 'elasticsearch'; | |||
import type { estypes } from '@elastic/elasticsearch'; |
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.
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) { |
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.
won't this not run if response.version_conflicts === 0
since thats a falsy value?
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.
Correct, a value of 0
never satisfied this condition.
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.
I wasn't sure if you wanted it to now given the defaults below (should have added that point). 👍🏾
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.
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) { |
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.
👍🏾
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @rylnd |
…' 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 <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…' 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 <[email protected]> Co-authored-by: Ryland Herrick <[email protected]>
…' 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 <[email protected]>
…-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
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