-
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 7.16-canary.7 #117305
Conversation
Pinging @elastic/kibana-core (Team:Core) |
@@ -87,7 +87,7 @@ export const fetchNumericFieldStats = async ( | |||
); | |||
const { body } = await esClient.search(request); | |||
|
|||
const aggregations = body.aggregations as { | |||
const aggregations = body.aggregations as unknown as { |
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.
tsc
complains sample
doesn't exist on Record<string, AggregationsAggregate>
. We need to refactor the way elasticsearch-js
declares `aggregations type. A first proposal is here elastic/elasticsearch-js#1596
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.
Very likely this new aggregations work will land only in v8.
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.
it can create some merge conflicts during a bug fix backporting, but let's see
@@ -315,11 +315,11 @@ export async function bulkUpdateAgents( | |||
}); | |||
|
|||
return { | |||
items: res.body.items.map((item: estypes.BulkResponseItemContainer) => ({ | |||
// @ts-expect-error ErrorCause is not assignable to Error | |||
items: res.body.items.map((item) => ({ |
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'm not sure these typings are correct. @joshdover
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.
@nchaulet I believe you're more familiar with this code. Should we be wrapping the item.update!.error
with new 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.
Looking at the code looks like we never consume these errors, I am going to investigate a little more what error handling we are missing, but it should not block that PR.
@elasticmachine merge upstream |
@@ -85,11 +85,13 @@ export const initLogSourceConfigurationRoutes = ({ framework, sources }: InfraBa | |||
? sources.updateSourceConfiguration( | |||
requestContext.core.savedObjects.client, | |||
sourceId, | |||
// @ts-ignore |
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've already added the same instructions during #113950
The problem is that an interface incompatibility error is highly unstable. scripts/type_check
might fail locally, but pass on CI
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
Summary
closes #117285