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

Expose Elasticsearch from start and deprecate from setup #59886

Merged
merged 4 commits into from
Mar 13, 2020

Conversation

rudolf
Copy link
Contributor

@rudolf rudolf commented Mar 11, 2020

Summary

As the first step of #55397 we deprecate the CoreSetup.elasticsearch API and expose CoreStart.elasticsearch instead.

We didn't previously feel like it's a high-enough priority to justify working on it, but since we're anyway making a breaking change to the API I thought it might be a good time to unify the admin and data clients as per #49870 and only expose elasticsearch.client and elasticsearch.createClient

Dev Docs

To remove any API that could allow to access/query savedObjects from core setup contract, and move them to the start contract instead. Deprecate the CoreSetup.elasticsearch API and expose CoreStart.elasticsearch instead.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@rudolf rudolf added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform labels Mar 11, 2020
@elasticmachine
Copy link
Contributor

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

@rudolf rudolf force-pushed the core-start-elasticsearch branch from f7d9867 to d3426d6 Compare March 11, 2020 11:25
@rudolf rudolf marked this pull request as ready for review March 11, 2020 11:25
@rudolf rudolf requested a review from a team as a code owner March 11, 2020 11:25
@rudolf rudolf added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.7.0 v8.0.0 labels Mar 11, 2020
public async start() {
if (typeof this.adminClient === 'undefined' || typeof this.createClient === 'undefined') {
throw new Error('ElasticsearchService needs to be setup before calling start');
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unnecessary else

return {
legacy: { config$: clients$.pipe(map(clients => clients.config)) },

adminClient,
dataClient,
esNodesCompatibility$,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't move to start as well? ok, probably after removing adminClient & dataClient

@@ -43,6 +43,7 @@ export interface ElasticsearchServiceSetup {
* const client = elasticsearch.createCluster('my-app-name', config);
* const data = await client.callAsInternalUser();
* ```
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment what is the blessed way to use cluster from now on?

throw new Error('ElasticsearchService needs to be setup before calling start');
} else {
return {
client: this.adminClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we reserve these property names for an elasticsearch-js compatible client? #35508
We can group them under legacy namespace, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, this is a good time to prepare for that. It could be confusing if there's a "legacy" client, but we won't provide any alternative until 8.0, but I don't have a better suggestion. It would save a lot of refactoring and risk if we can prevent it, so I think it's worth the possible short term confusion for the long term benefits.

Copy link
Contributor

@mshustov mshustov Mar 13, 2020

Choose a reason for hiding this comment

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

but we won't provide any alternative until 8.0,

why not? I'd expect it to land in v7.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I somehow missed that!

@@ -49,6 +49,7 @@ export interface InternalCoreSetup {
*/
export interface InternalCoreStart {
capabilities: CapabilitiesStart;
elasticsearch: ElasticsearchServiceStart;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we migrate the platform code to the new API? I see that SO still uses the deprecated one. It can be done as a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

And we should update the migration guide accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we migrate the platform code to the new API? I see that SO still uses the deprecated one. It can be done as a follow-up.

I've added a task to #55397

@rudolf rudolf requested a review from mshustov March 13, 2020 09:54
Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

I can see that our migration guide and examples still reference adminClient& dataClient. Added as follow-ups to #55397

@rudolf rudolf merged commit 6e6325b into elastic:master Mar 13, 2020
@rudolf rudolf deleted the core-start-elasticsearch branch March 13, 2020 20:17
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 14, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 16, 2020
…o alerting/view-in-app

* 'alerting/view-in-app' of github.com:gmmorris/kibana: (33 commits)
  [SIEM] Adds 'Closes and opens signals' Cypress test (elastic#59950)
  [NP] Graph migration (elastic#59409)
  [ML] Clone analytics job  (elastic#59791)
  Move VALUE_CLICK_TRIGGER and APPLY_FILTER_TRIGGER to ui_action… (elastic#60202)
  Handle improperly defined Watcher Logging Action text parameter. (elastic#60169)
  Move select range trigger to uiActions (elastic#60168)
  [SIEM][CASES] Configure cases: Final (elastic#59358)
  Closes elastic#59784. Sets xpack.apm.serviceMapEnabled default value true. (elastic#60153)
  [siem/cypress] update junit filename to be picked up by runbld (elastic#60156)
  OSS logic added to test environment  (elastic#60134)
  Move canvas setup into app mount (elastic#59926)
  enable triggers_actions_ui plugin by default (elastic#60137)
  Expose Elasticsearch from start and deprecate from setup (elastic#59886)
  [SIEM] [Case] Insert timeline bugfix and limit 25 cases (elastic#60136)
  [ML] Client side cut over (elastic#60100)
  Disable query bar on service map routes (elastic#60118)
  Move subscribe_with_scope to kibana_legacy (elastic#59781)
  [Ingest] Agent Config create with sys monitoring (elastic#60111)
  [Watcher UI] Not possible to edit a watch that was created with the API if the ID contains a dot (elastic#59383)
  Actually fetch functionbeat fields needed for telemetry (elastic#60054)
  ...
rudolf added a commit that referenced this pull request Mar 16, 2020
…0163)

* Expose Elasticsearch from start and deprecate from setup

* Expose client under legacy namespace, add deprecated note

* Update migration guide

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 16, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 16, 2020
* master: (40 commits)
  skips 'config_open.ts' files from linter check (elastic#60248)
  [Searchprofiler] Spacing between rendered shards (elastic#60238)
  Add UiSettings validation & Kibana default route redirection (elastic#59694)
  [SIEM][CASE] Change configuration button (elastic#60229)
  [Step 1][App Arch] Saved object migrations from kibana plugin to new platform  (elastic#59044)
  adds new test (elastic#60064)
  [Uptime] Index Status API to Rest (elastic#59657)
  [SIEM] Adds 'Closes and opens signals' Cypress test (elastic#59950)
  [NP] Graph migration (elastic#59409)
  [ML] Clone analytics job  (elastic#59791)
  Move VALUE_CLICK_TRIGGER and APPLY_FILTER_TRIGGER to ui_action… (elastic#60202)
  Handle improperly defined Watcher Logging Action text parameter. (elastic#60169)
  Move select range trigger to uiActions (elastic#60168)
  [SIEM][CASES] Configure cases: Final (elastic#59358)
  Closes elastic#59784. Sets xpack.apm.serviceMapEnabled default value true. (elastic#60153)
  [siem/cypress] update junit filename to be picked up by runbld (elastic#60156)
  OSS logic added to test environment  (elastic#60134)
  Move canvas setup into app mount (elastic#59926)
  enable triggers_actions_ui plugin by default (elastic#60137)
  Expose Elasticsearch from start and deprecate from setup (elastic#59886)
  ...
rudolf added a commit to rudolf/kibana that referenced this pull request Mar 17, 2020
@kibanamachine
Copy link
Contributor

💔 Build Failed


Test Failures

Kibana Pipeline / kibana-intake-agent / Jest Integration Tests.packages/kbn-plugin-generator/integration_tests.running the plugin-generator via 'node scripts/generate_plugin.js plugin-name' with default config then running with es instance 'yarn start' should result in the spec plugin being initialized on kibana's stdout

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 4 times on tracked branches: https://github.com/elastic/kibana/issues/54481


Stack Trace

Error: Command failed with exit code 126: ./bin/elasticsearch-keystore create
    at makeError (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/execa/lib/error.js:56:11)
    at handlePromise (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/execa/index.js:114:26)
    at process._tickCallback (internal/process/next_tick.js:68:7)

History

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

rudolf added a commit that referenced this pull request Apr 15, 2020
* x-pack/watcher: use Elasticsearch from CoreStart

* x-pack/upgrade_assistant: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/lens: use Elasticsearch from CoreStart

* expressions: use Elasticsearch from CoreStart

* x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup

* x-pack/oss_telemetry: use Elasticsearch from CoreStart

* Cleanup after #59886

* x-pack/watcher: create custom client only once

* Revert "x-pack/watcher: create custom client only once"

This reverts commit 78fc4d2.

* Revert "x-pack/watcher: use Elasticsearch from CoreStart"

This reverts commit b621af9.

* x-pack/task_manager: use Elasticsearch from CoreStart

* x-pack/event_log: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/apm: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* PR Feedback

* APM review nits

* Remove unused variable

* Remove unused variable

* x-pack/apm: better typesafety

Co-authored-by: Elastic Machine <[email protected]>
wayneseymour pushed a commit that referenced this pull request Apr 15, 2020
* x-pack/watcher: use Elasticsearch from CoreStart

* x-pack/upgrade_assistant: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/lens: use Elasticsearch from CoreStart

* expressions: use Elasticsearch from CoreStart

* x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup

* x-pack/oss_telemetry: use Elasticsearch from CoreStart

* Cleanup after #59886

* x-pack/watcher: create custom client only once

* Revert "x-pack/watcher: create custom client only once"

This reverts commit 78fc4d2.

* Revert "x-pack/watcher: use Elasticsearch from CoreStart"

This reverts commit b621af9.

* x-pack/task_manager: use Elasticsearch from CoreStart

* x-pack/event_log: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/apm: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* PR Feedback

* APM review nits

* Remove unused variable

* Remove unused variable

* x-pack/apm: better typesafety

Co-authored-by: Elastic Machine <[email protected]>
rudolf added a commit to rudolf/kibana that referenced this pull request Apr 23, 2020
* x-pack/watcher: use Elasticsearch from CoreStart

* x-pack/upgrade_assistant: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/lens: use Elasticsearch from CoreStart

* expressions: use Elasticsearch from CoreStart

* x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup

* x-pack/oss_telemetry: use Elasticsearch from CoreStart

* Cleanup after elastic#59886

* x-pack/watcher: create custom client only once

* Revert "x-pack/watcher: create custom client only once"

This reverts commit 78fc4d2.

* Revert "x-pack/watcher: use Elasticsearch from CoreStart"

This reverts commit b621af9.

* x-pack/task_manager: use Elasticsearch from CoreStart

* x-pack/event_log: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/apm: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* PR Feedback

* APM review nits

* Remove unused variable

* Remove unused variable

* x-pack/apm: better typesafety

Co-authored-by: Elastic Machine <[email protected]>
rudolf added a commit that referenced this pull request Apr 24, 2020
#64329)

* Refactor Plugins to access elasticsearch from CoreStart (#59915)

* x-pack/watcher: use Elasticsearch from CoreStart

* x-pack/upgrade_assistant: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/lens: use Elasticsearch from CoreStart

* expressions: use Elasticsearch from CoreStart

* x-pack/remote_clusters: remove unused Elasticsearch dependency on CoreSetup

* x-pack/oss_telemetry: use Elasticsearch from CoreStart

* Cleanup after #59886

* x-pack/watcher: create custom client only once

* Revert "x-pack/watcher: create custom client only once"

This reverts commit 78fc4d2.

* Revert "x-pack/watcher: use Elasticsearch from CoreStart"

This reverts commit b621af9.

* x-pack/task_manager: use Elasticsearch from CoreStart

* x-pack/event_log: use Elasticsearch from CoreStart

* x-pack/alerting: use Elasticsearch from CoreStart

* x-pack/apm: use Elasticsearch from CoreStart

* x-pack/actions: use Elasticsearch from CoreStart

* PR Feedback

* APM review nits

* Remove unused variable

* Remove unused variable

* x-pack/apm: better typesafety

Co-authored-by: Elastic Machine <[email protected]>

* Fix event log tests

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants