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

Deprecate es API exposed from setup contract #67596

Merged
merged 12 commits into from
Jun 1, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented May 28, 2020

Summary

Rename remaining elasticsearch API exposed from setup contract. Unfortunately, we cannot remove all usages right now. Closes #55397
Also fixes #49870 removing adminClient & dataClient to inline with start contract.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Dev Docs

Elasticsearch API exposed from setup contract is not available and will be deleted without any notice. Please, switch to core start API instead.

// before
setup(core: CoreSetup) {
   core.elasticsearch.dataClient(...)
   core.elasticsearch.adminClient(...)
}
// after
setup(core: CoreSetup) {
   core.elasticsearch.legacy.client(...)
}

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.9.0 labels May 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@mshustov mshustov marked this pull request as ready for review May 28, 2020 15:58
@mshustov mshustov requested review from a team as code owners May 28, 2020 15:58
@mshustov mshustov requested a review from a team May 28, 2020 15:58
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

security changes LGTM

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

ML edit LGTM

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Unfortunately, we cannot remove all usages right now.

Where are we tracking the remaining usages? Maybe we could consolidate the list in #55397 (comment) to make this a bit easier to scan.

src/core/server/elasticsearch/elasticsearch_service.ts Outdated Show resolved Hide resolved
src/core/server/elasticsearch/types.ts Show resolved Hide resolved
readonly dataClient: IClusterClient;
/**
* @deprecated
* Use {@link ElasticsearchServiceStart.legacy | ElasticsearchServiceStart.legacy.client} instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I don't think this line applies anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove the comment when the new elasticsearch client merged. Till that moment, we'd prefer plugins to use start contract. Wouldn't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I misread this. I thought it was copied over from dataClient. 👍

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Changes to Remote Clusters mocks LGTM.

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM for stack monitoring

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

Telemetry changes LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@mshustov mshustov merged commit 53b9542 into elastic:master Jun 1, 2020
@mshustov mshustov deleted the issue-55397-deprecate-es-from-setup branch June 1, 2020 14:16
@mshustov
Copy link
Contributor Author

mshustov commented Jun 1, 2020

waiting for #67818 to backport the changes

@joelgriffith
Copy link
Contributor

👋 I'm working on debugging what happened to our tests in this backport, I'll chime in once things are green/merged

mshustov added a commit to mshustov/kibana that referenced this pull request Jun 2, 2020
* move elasticsearch client under legacy namespace

* update mocks and tests

* update platform code

* update legacy code

* update plugins using elasticsearch setup API

* update request handler context

* update docs

* rename remaining places

* address comments

* fix merge conflict error
mshustov added a commit that referenced this pull request Jun 2, 2020
* move elasticsearch client under legacy namespace

* update mocks and tests

* update platform code

* update legacy code

* update plugins using elasticsearch setup API

* update request handler context

* update docs

* rename remaining places

* address comments

* fix merge conflict error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.9.0
Projects
None yet