-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
Pinging @elastic/ml-ui (:ml) |
/** | ||
* @apiGroup SystemRoutes | ||
* | ||
* @api {post} /api/ml/ml_capabilities Check ML capabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be get
There was a problem hiding this comment.
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', { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be get
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be get
There was a problem hiding this comment.
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'), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 608f48d
There was a problem hiding this 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 😄
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ⚡️
* [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
* [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
Summary
Part of #49743. Updates system routes to use the new platform router.
Checklist