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 es config on es setup contract #73055

Merged
merged 3 commits into from
Aug 17, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Jul 23, 2020

Summary

Required for #68497

Expose legacy.config$ on ES service setup contract

Checklist

Comment on lines 37 to 42
/**
* Provide direct access to the current elasticsearch configuration.
*
* @deprecated this will be removed in a later version.
*/
readonly config$: Observable<ElasticsearchConfig>;
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 could have kept legacy.config$ and expose it on the public contract instead, but now that this is going to be consumed by KP plugins, I thought it was better on top level. Also, that would allow to totally get rid of esSetup.legacy when we remove the legacy client

  • flagged it as deprecated. Not sure if it's the right move.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep it under legacy namespace to make it even more explicit that we are going to remove it. @joshdover any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Deprecation tags are easy to miss (they're really only present in intellisense right now since we don't have any linter warnings for using deprecated APIs). Given that, I also prefer keeping it under the legacy namespace to make it clear that this will be removed in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related, this new feature in TS v4 / VSCode will make deprecated APIs more apparent: https://devblogs.microsoft.com/typescript/announcing-typescript-4-0-rc/#deprecated-support

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Jul 23, 2020
@pgayvallet pgayvallet marked this pull request as ready for review July 23, 2020 16:28
@pgayvallet pgayvallet requested a review from a team as a code owner July 23, 2020 16:28
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@pgayvallet pgayvallet merged commit 57a802c into elastic:master Aug 17, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Aug 17, 2020
* expose es config on es setup contract

* move config$ back to legacy
pgayvallet added a commit that referenced this pull request Aug 18, 2020
* expose es config on es setup contract

* move config$ back to legacy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants