-
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
Update ES client to 8.1.0-canary.2 #119791
Conversation
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.
Viz editors changes LGTM!
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_solution
changes LGTM. I only have a few suggestions for removing redundant union types that use undefined
.
sortOrder?: estypes.SearchSortOrder; | ||
searchAfterSortIds: estypes.SearchSortResults | undefined; | ||
sortOrder?: estypes.SortOrder; | ||
searchAfterSortIds: estypes.SortResults | undefined; |
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.
searchAfterSortIds: estypes.SortResults | undefined; | |
searchAfterSortIds?: estypes.SortResults; |
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.
@ashokaditya Happy to accept those changes. However, IMO, they are not exactly the same:
AFAIK, searchAfterSortIds: estypes.SortResults | undefined;
enforces the property to be present but it accepts undefined. Setting a property as optional means that the property might not be provided.
It's a small difference, but depending on where it's used, it can catch some errors.
If you're happy with making the property optional, I'll gladly accept your suggestion :)
EDIT: I noticed about the agreement with Pierre. I also agree that it's better to make these changes in a separate PR.
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.
@afharo yes quite right. At least in these cases, the types look like they are optional. Although it does make sense to explicitly define this so that the param is explicitly assigned undefined
when that method is invoked if that is what is intended 👍
x-pack/plugins/security_solution/server/lib/detection_engine/signals/single_search_after.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/signals/utils.ts
Show resolved
Hide resolved
...ty_solution/server/search_strategy/security_solution/factory/cti/event_enrichment/helpers.ts
Show resolved
Hide resolved
@@ -59,6 +59,7 @@ export default function ApiTest({ getService }: FtrProviderContext) { | |||
`); | |||
expectSnapshot( | |||
response.body.traceDocs.map((doc) => | |||
// @ts-expect-error Property 'processor' does not exist on type 'Profile'. |
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.
this (and the other @ts-expect-error before) indicate that the request body is not valid. I'll have a look.
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.
Approving on behalf of APM UI, @walterra will also have a look at the changes in APM's correlations code (thank you Walter!)
Relaunching the tests because it looks like they fail due to timeouts in Buildkite (cc @elastic/kibana-operations) |
@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.
Uptime change LGTM!
x-pack/plugins/security_solution/common/endpoint/data_loaders/index_fleet_server.ts
Outdated
Show resolved
Hide resolved
…index_fleet_server.ts Co-authored-by: Steph Milovic <[email protected]>
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 for threat hunting, thank you
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.
Transforms/ML/APM Correlations LGTM
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.
Transforms/ML/APM Correlations LGTM
@elasticmachine merge upstream |
I double-checked the changes for the 2 pending reviewers. The changes are minimal:
I'll merge after green CI. |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: cc @afharo |
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
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 ✨ 🚀 🌔
Security telemetry changes look good!
Summary
Closes #119454
Closes #116111
Important changes:
_type
meta field is removed Remove support for _type in searches elasticsearch#68564TAggregations
generic Aggregations should be a generic in responses elasticsearch-js#1596asStream
configuration option Add asStream support elastic-transport-js#34Aggregations
In this PR, I declared definitions as
I use
extends
to enforcebuckets
compatbility. ES client type definitions definebuckets
type asso we have to specify explicitly the bucket type we expect - object or array-based.
Hopefully, in the future, we will be able to infer a response type from a request type. For now, we declare it explicitly.