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

[ML] New Platform server shim: update system routes #57835

Merged
merged 6 commits into from
Feb 19, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Feb 18, 2020

Summary

Part of #49743. Updates system routes to use the new platform router.

Checklist

  • Documentation was added for features that require explanation or tutorials

@darnautov darnautov added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 18, 2020
@darnautov darnautov requested a review from a team as a code owner February 18, 2020 12:17
@darnautov darnautov self-assigned this Feb 18, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

/**
* @apiGroup SystemRoutes
*
* @api {post} /api/ml/ml_capabilities Check ML capabilities
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}: RouteInitialization) {
async function getNodeCount(context: RequestHandlerContext) {
const filterPath = 'nodes.*.attributes';
const resp = await context.ml!.mlClient.callAsInternalUser('nodes.info', {
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 Feb 18, 2020

Choose a reason for hiding this comment

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

This call to nodes.info was originally being done with callWithInternalUser from callWithInternalUserFactory - this should be replaced by the NP equivalent available on context -> context.core.elasticsearch.adminClient.callAsInternalUser

https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md#server-side (there's a small typo where core is missing but I'm updating it to the correct path in another PR)

Since context.ml!.mlClient.callAsInternalUser and context.core.elasticsearch.adminClient.callAsInternalUser are indeed the same - this is fine 👌

/**
* @apiGroup SystemRoutes
*
* @api {post} /api/ml/ml_node_count Get the amount of ML nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* @apiGroup SystemRoutes
*
* @api {post} /api/ml/info Get ML info
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

licensePreRoutingFactory(xpackMainPlugin, async (context, request, response) => {
try {
return response.ok({
body: await context.ml!.mlClient.callAsCurrentUser('search'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This call to search was originally callWithRequest('search', request.payload) so it requires that request.body be passed here as well. This means that validate will need to be updated with a schema validation.

Copy link
Contributor Author

@darnautov darnautov Feb 18, 2020

Choose a reason for hiding this comment

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

Ah sorry, but I don't want to write validation for elasticsearch search endpoint 😅
And I hope we are going to get rid of this endpoint very soon.
Thanks for catching the missing body param!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 608f48d

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! Just left a couple of comments 😄

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡️

@darnautov darnautov merged commit 4fa5942 into elastic:master Feb 19, 2020
@darnautov darnautov deleted the ML-49743-system branch February 19, 2020 13:41
darnautov added a commit to darnautov/kibana that referenced this pull request Feb 19, 2020
* [ML] NP system routes

* [ML] apidoc.json

* [ML] address PR comments

* [ML] fix apidoc methods, passing es_search endpoint payload

* [ML] add dummy body validation for es_search, fix ignoreSpaces query param

* [ML] _has_privileges validate body
darnautov added a commit that referenced this pull request Feb 20, 2020
* [ML] NP system routes

* [ML] apidoc.json

* [ML] address PR comments

* [ML] fix apidoc methods, passing es_search endpoint payload

* [ML] add dummy body validation for es_search, fix ignoreSpaces query param

* [ML] _has_privileges validate body
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration :ml release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants