-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Ingest Pipelines] Migrate to new ES client #96406
[Ingest Pipelines] Migrate to new ES client #96406
Conversation
- removed use of isEsError to detect legacy errors - refactored types to use types from @elastic/elasticsearch instead (where appropriate) tested get, put, post, delete, simulate and documents endpoints locally
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@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.
Thanks for migrating another plugin @jloleysens! I left two comments that I'd like to see addressed before approving.
Would you also mind updating x-pack/test/api_integration/apis/management/ingest_pipelines/lib/elasticsearch.ts
to use the new es-js client?
if (isEsError(error)) { | ||
// ES returns 404 when there are no pipelines | ||
// Instead, we return an empty array and 200 status back to the client | ||
if (error.status === 404) { |
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.
Is this check no longer needed?
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.
Good point! This behaviour needs to be preserved!
|
||
try { | ||
const pipelines = await callAsCurrentUser('ingest.getPipeline'); | ||
const { body: pipelines } = await clusterClient.asCurrentUser.ingest.getPipeline({ |
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.
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.
Haha this is what happens when you commit test code! This is how I came across the issue with messages returned in handlEsError
😅 . Will fix!
@elasticmachine merge upstream |
@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.
Latest LGTM. Thanks @jloleysens!
I still see that x-pack/test/api_integration/apis/management/ingest_pipelines/lib/elasticsearch.ts
is using the legacy client. I'd prefer that is addressed as part of this PR so we don't have to go back and fix it. I'm going to go ahead and approve to unblock you, with the assumption that that will be updated before merging 😄 .
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* - migrated use of legacy.client to client - removed use of isEsError to detect legacy errors - refactored types to use types from @elastic/elasticsearch instead (where appropriate) tested get, put, post, delete, simulate and documents endpoints locally * remove use of legacyEs service in functional test * fixing type issues and API response object * remove id from get all request! * reinstated logic for handling 404 from get all pipelines request * clarify error handling with comments and small variable name refactor * updated delete error responses * update functional test * refactor use of legacyEs Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
See #73973
How to test
Steps I followed:
.kibana
index, you should just retrieve a corresponding doc id too)