[Snapshot Restore] Migrate to new ES client #95499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jloleysens
added
v8.0.0
Team:Kibana Management
|
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
- updated routes in light of new responses
There was a problem hiding this comment.
Nice work @jloleysens! It's great to see another plugin migrated to use the new es-js client ✅
I did not find any regressions. I went through the flow of creating a repo, policy, snapshot, and then restoring a snapshot. I also verified the delete actions, running retention, and cloud-managed repos/policies.
There are a few instances where the types that come from the new ES client differ from the types created for the original behaviour. Updating the types will, in some instances, require functionality changes (like handling types that are optional) and these are not updated in this PR.
Were you planning to address this separately? If not, one thought I had was to add these items to #95241 so we don't lose track.
I left one comment in the PR regarding error handling. I think this applies to all routes, but I did not leave a comment for each one. Let me know what you think!
There are a few other things I noticed that I'd like to see addressed before merging:
- I think there's still some code in
plugin.tsrelated to thesnapshotRestoreESClientthat can be removed. - We should be able to delete
client/elasticsearch_sr.tsnow 🎉 - Would you mind updating the API integration tests as well? There are details on how to handle this in Stack Management elasticsearch-js client migration #73973 under the section "API integration tests"
| statusCode: e.statusCode, | ||
| body: e, | ||
| }); | ||
| return handleEsError({ error: e, response: res }); |
There was a problem hiding this comment.
I don't think you need the isEsError check on line 89; handleEsError should handle that for you, and throw if not.
There was a problem hiding this comment.
Right! I'll simplify the error handling a little bit!
| }); | ||
|
|
||
| // TODO: remove this "as unknown" workaround when the types for this endpoint are correct | ||
| // See https://github.com/elastic/elasticsearch-js/issues/1427 |
There was a problem hiding this comment.
Per #83808, I think any incorrect types should be muted with @ts-expect-error @elastic/elasticsearch
If you spot any problems in the imported typings, please mute them with @ts-expect-error @elastic/elasticsearch instruction and report them to https://github.com/elastic/elastic-client-generator/issues/new/choose
Once the type errors are fixed in the upstream library, Kibana will receive the update, and @ts-expect-error @elastic/elasticsearch should be deleted.
| repository, | ||
| snapshot: '_all', | ||
| ignore_unavailable: true, // Allow request to succeed even if some snapshots are unavailable. | ||
| }); | ||
|
|
||
| // TODO: remove this "as unknown" workaround when the types for this endpoint are correct | ||
| // See https://github.com/elastic/elasticsearch-js/issues/1427 |
There was a problem hiding this comment.
See comment above about incorrect types in the es-js client
| repository, | ||
| snapshot: '_all', | ||
| ignore_unavailable: true, | ||
| }); | ||
|
|
||
| // TODO: remove this "as unknown" workaround when the types for this endpoint are correct |
There was a problem hiding this comment.
See comment above about incorrect types in the es-js client
|
@elasticmachine merge upstream |
|
Thanks for the review @alisonelizabeth !
I was planning on addressing these separately, yes. I opened this issue to make the client lib aware: elastic/elasticsearch-specification#250 (at least for the cases where the lib types are incorrect). I'll add a reference to snapshot and restore to that issue too!
Initially I was planning on doing this in a separate PR, but I see the required changes should be quite small! I'll include them here! |
|
@alisonelizabeth I think I addressed your comments, would you mind taking another look? |
alisonelizabeth
approved these changes
Mar 30, 2021
|
@elasticmachine merge upstream |
17 tasks
|
@jloleysens I just remembered there are some differences with the snapshot APIs on |
|
Thanks for calling that out @alisonelizabeth ! Depending on the nature of the differences the backport might need another round of review. I'll take a look :) |
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Mar 31, 2021
* wip, migrated routes and plugins * refactored all ES error handling to use handleEsError and new isEsError detection * - fixed Jest tests for new es client - updated routes in light of new responses * remove unused import * remove unecessary isEsError check in rest api route handlers * mute all incorrect types from client lib using @ts-expect-error * reordered and clean up imports, removed legacy client code * update legacy test runner * updated use of legacyES Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/snapshot_restore/server/routes/api/repositories.ts # x-pack/plugins/snapshot_restore/server/routes/api/snapshots.test.ts # x-pack/plugins/snapshot_restore/server/routes/api/snapshots.ts
74 tasks
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/uptime/public/pages.settings form it show select a connector flyoutStandard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/dashboard/dashboard_filtering·ts.dashboard app using current data dashboard filtering using a pinned filter that excludes all data "before all" hook for "filters on pie charts"Standard OutStack TraceKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/dashboard/dashboard_filtering·ts.dashboard app using current data dashboard filtering using a pinned filter that excludes all data "after all" hook for "vega is filtered"Standard OutStack Traceand 1 more failures, only showing the first 3. Metrics [docs]
History
To update your PR or re-run it, just comment with: |
jloleysens
added a commit
that referenced
this pull request
Mar 31, 2021
* [Snapshot Restore] Migrate to new ES client (#95499) * wip, migrated routes and plugins * refactored all ES error handling to use handleEsError and new isEsError detection * - fixed Jest tests for new es client - updated routes in light of new responses * remove unused import * remove unecessary isEsError check in rest api route handlers * mute all incorrect types from client lib using @ts-expect-error * reordered and clean up imports, removed legacy client code * update legacy test runner * updated use of legacyES Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/snapshot_restore/server/routes/api/repositories.ts # x-pack/plugins/snapshot_restore/server/routes/api/snapshots.test.ts # x-pack/plugins/snapshot_restore/server/routes/api/snapshots.ts * update use of es client to conform to 7.x responses * reinstated 404 response when snapshot is not found
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
See #73973
Notes
There are a few instances where the types that come from the new ES client differ from the types created for the original behaviour. Updating the types will, in some instances, require functionality changes (like handling types that are optional) and these are not updated in this PR.