Skip to content
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

[Rollup] Migrate to new ES client #95926

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Mar 31, 2021

Summary

See #73973

How to test

  1. Start Kibana and ES
  2. Navigate to the rollup job sub section in the management section
  3. Create a rollup using the wizard (make sure that pattern validation check is working correctly)
  4. Delete a rollup job

@jloleysens jloleysens added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.13.0 labels Mar 31, 2021
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens jloleysens marked this pull request as ready for review April 6, 2021 13:49
@jloleysens jloleysens requested a review from a team as a code owner April 6, 2021 13:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

indexPattern: '_all',
}
);
const { client: clusterClient } = context.core.elasticsearch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not sure why we need to rename client here.

@@ -71,16 +67,12 @@ export const registerValidateIndexPatternRoute = ({
},
},
license.guardApiRoute(async (context, request, response) => {
const { client: clusterClient } = context.core.elasticsearch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same nit here :)

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jloleysens, thanks a lot for migrating rollup jobs to the new client!
I tested locally and everything worked for me. Code changes LGTM too, just left one nit in the comments about renaming when object destructuring.

@jloleysens jloleysens merged commit 4d43a4f into elastic:master Apr 7, 2021
@jloleysens jloleysens deleted the rollup-jobs/migrate-to-new-es-client branch April 7, 2021 12:49
jloleysens added a commit that referenced this pull request Apr 8, 2021
* initial pass at es client migration

* fixed potential for not passing in an error message and triggering an unhandled exception

* reworked ad hoc fixing of error response

* delete legacy client file and remove use of legacyEs service

* remove unused import

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	x-pack/plugins/rollup/server/routes/api/jobs/register_start_route.ts

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@alisonelizabeth alisonelizabeth added Feature:Rollups and removed Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI labels May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Rollups release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants