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

Use new ES client for licensing plugin #92143

Merged

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Feb 22, 2021

Summary

Part of #83910
Fix #92185

  • migrate the licensing plugin to use the new ES client
  • adapt the monitoring usage of createLicensePoller

Checklist

@pgayvallet pgayvallet changed the title Add licensing plugin status change Use new ES client for licensing plugin Feb 22, 2021
@pgayvallet pgayvallet added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.14.0 release_note:skip Skip the PR/issue when compiling release notes labels Jun 23, 2021
Comment on lines -37 to +46
function normalizeServerLicense(license: RawLicense): PublicLicense {
function normalizeServerLicense(
license: estypes.XpackInfoMinimalLicenseInformation
): PublicLicense {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The client now provides proper typing for the xpack/info endpoint, so we can now use them.

Comment on lines +185 to +186
const normalizedLicense =
response.license && response.license.type !== 'missing'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL there is a missing type in the client def. Not sure this is really used, as it seems we never encountered it, but still added the check.

Comment on lines 192 to 196
const cluster = instantiateClient(
config.ui.elasticsearch,
this.log,
core.elasticsearch.createClient
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As monitoring is not migrated to the new client yet (due to this issue), I instantiated the new client at the only place we currently need it. I will let the owners refactor that when migrating the rest of the plugin

@pgayvallet pgayvallet marked this pull request as ready for review June 23, 2021 15:06
@pgayvallet pgayvallet requested a review from a team as a code owner June 23, 2021 15:06
@pgayvallet pgayvallet requested a review from a team June 23, 2021 15:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@matschaffer
Copy link
Contributor

@pgayvallet it looks like @simianhacker has the monitoring ui parts of this covered in #101850

Guessing it'd make sense for you to back yours out of this PR, but maybe best for you and him to work that out.

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Jun 24, 2021

Guessing it'd make sense for you to back yours out of this PR, but maybe best for you and him to work that out.

Thanks for the info. Looking at #101850, it would be pretty straightforward to rebase my PR against it once it gets merged. I guess the best option is for me to wait until #101850 is merged and just remove the shim that @simianhacker was forced to use the your licensing service.

Given that the PR is approved, I'm guessing it should be merged soon?

path: '/_xpack?accept_enterprise=true',
const { body: response } = await client.asInternalUser.xpack.info({
// @ts-expect-error `accept_enterprise` is not present in the client definition
accept_enterprise: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

while license.get accepts accept_enterprise: true, but it doesn't contain features in the response object 😞

x-pack/plugins/licensing/server/plugin.ts Show resolved Hide resolved
@simianhacker simianhacker self-requested a review June 24, 2021 15:43
Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

Monitoring portion looks good to me.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
licensing 44 42 -2
Unknown metric groups

API count

id before after diff
licensing 120 117 -3

References to deprecated APIs

id before after diff
licensing 18 0 -18
monitoring 2 0 -2
total -20

History

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

@pgayvallet pgayvallet merged commit 5bb9691 into elastic:master Jun 30, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Jun 30, 2021
* use new client for licensing API

* add logs

* adapt unit tests

* Revert "add logs"

This reverts commit 4a61b64

* fix some type errors

* fix test types

* adapt monitoring usage of `createLicensePoller`

* remove test comment

* fix unit test

* remove createLicensePoller from setup contract

* fix unit tests
@pgayvallet
Copy link
Contributor Author

Merging after FF to complete #101850

pgayvallet added a commit that referenced this pull request Jun 30, 2021
* use new client for licensing API

* add logs

* adapt unit tests

* Revert "add logs"

This reverts commit 4a61b64

* fix some type errors

* fix test types

* adapt monitoring usage of `createLicensePoller`

* remove test comment

* fix unit test

* remove createLicensePoller from setup contract

* fix unit tests
spalger pushed a commit to spalger/kibana that referenced this pull request Jul 7, 2021
* use new client for licensing API

* add logs

* adapt unit tests

* Revert "add logs"

This reverts commit 4a61b64

* fix some type errors

* fix test types

* adapt monitoring usage of `createLicensePoller`

* remove test comment

* fix unit test

* remove createLicensePoller from setup contract

* fix unit tests
spalger pushed a commit that referenced this pull request Jul 7, 2021
* use new client for licensing API

* add logs

* adapt unit tests

* Revert "add logs"

This reverts commit 4a61b64

* fix some type errors

* fix test types

* adapt monitoring usage of `createLicensePoller`

* remove test comment

* fix unit test

* remove createLicensePoller from setup contract

* fix unit tests

Co-authored-by: Pierre Gayvallet <pierre.gayvallet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore 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.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitoring: legacy ES client usages blocking migration of the licensing plugin
7 participants