[ML] New Platform server shim: update recognize modules routes to use new platform router by darnautov · Pull Request #57206 · elastic/kibana

darnautov · 2020-02-10T15:23:39Z

Summary

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

Checklist

Documentation was added for features that require explanation or tutorials

elasticmachine · 2020-02-10T15:23:42Z

Pinging @elastic/ml-ui (:ml)

alvarezmelissa87 · 2020-02-10T20:09:18Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

I think it would be a good idea to name this this.callAsCurrentUser to be consistent with the other updated route models

alvarezmelissa87 · 2020-02-10T20:16:44Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

What do you think about moving this to a schema file to be consistent with the other converted routes?
There are also enough schema files in the new_platform directory that I think it warrants creating a schemas dir in there. I'm happy to do that part but would be good to get these body schemas in a separate file.

Moved to a dedicated file in 63ea75b

alvarezmelissa87 · 2020-02-10T20:33:21Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

Wouldn't some of these types be string?

Added types in 63ea75b

alvarezmelissa87 · 2020-02-10T20:47:10Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

It actually might be worth checking that this.savedObjectsClient exists here instead of using non-null assertion here. Even though right now loadIndexPatterns is only used after savedObjectsClient has been set this check would guard against any future updates/additions to this class that might try to use this method differently.

I believe it should be passed to the constructor, not the method. In that case, no check is required. I just wanted to avoid any code changes in this PR, but probably it worth a small refactoring.

Refactored in 63ea75b

alvarezmelissa87 · 2020-02-10T20:53:26Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

As mentioned above, might be worth checking that savedObjectsClient has been set.

alvarezmelissa87 · 2020-02-10T20:58:01Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

I'm not familiar with this - why is it needed?

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions
But I added const assertion here by mistake, will remove

alvarezmelissa87 · 2020-02-10T21:01:26Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

Would be good to know what is being ignored here - to be consistent

For some reason, the issue is numeral from @elastic/numeral. It complains that value prop does not exist.

alvarezmelissa87 · 2020-02-10T21:01:48Z

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts

jgowdyelastic · 2020-02-10T21:02:42Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

i'm unsure about using getSavedObjectsClient here as it uses the elevated internal user to create the saved object repository which could be a security risk.
The original version used the getSavedObjectsClient from the request which I imagine had the current user's level of privileges.
I'm unsure about this and I think is worth investigating.
Also, I didn't realise that the savedObjects being passed to the router are of the type SavedObjectsLegacyService.
I wonder if it's possible to use the non-legacy version of the saved objects client in our plugin right now, before we move it to the new platform proper.

Agreed 🤔 I think we should be to use NP's context.core.savedObjects.client which is meant to replace request.getSavedObjectsClient (https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md#server-side)

Since context is scoped like the previously used legacy request this should give us the same behavior, I'd think.

Fixed in 63ea75b

jgowdyelastic · 2020-02-11T09:14:25Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

prefix should be optional.
See documentation here:
https://github.com/elastic/ml-team/blob/master/ui/rest_api/modules/modules_api_setup.md

jgowdyelastic

One small change needed, but otherwise LGTM

jgowdyelastic · 2020-02-11T13:19:46Z

x-pack/legacy/plugins/ml/server/new_platform/modules.ts

+export const setupModuleBodySchema = schema.object({
+  prefix: schema.maybe(schema.string()),
+  groups: schema.maybe(schema.arrayOf(schema.string())),
+  indexPatternName: schema.string(),


indexPatternName needs to be optional

alvarezmelissa87

LGTM ⚡️

kibanamachine · 2020-02-11T16:05:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6d23884

History

💚 Build #25833 succeeded 9edbdec
💔 Build #25814 failed 63ea75b
💔 Build #25646 failed 1f975d4355655e07727daf75aab2e7abffbf0d59
💔 Build #25596 failed 3f6488a75c97e5d77a021843d2175bdc4d4d55d9

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

… new platform router (elastic#57206) * [ML] modules with NP router * [ML] use extendedRouteInitializationDeps * [ML] add apidoc annotations * [ML] address PR comments * [ML] convert data_recognizer test to jest * [ML] optional indexPatternName

… new platform router (#57206) (#57335) * [ML] modules with NP router * [ML] use extendedRouteInitializationDeps * [ML] add apidoc annotations * [ML] address PR comments * [ML] convert data_recognizer test to jest * [ML] optional indexPatternName

* master: (27 commits) Include actions new platform plugin for codeowners (elastic#57252) [APM][docs] 7.6 documentation updates (elastic#57124) Expressions refactor (elastic#54342) [ML] New Platform server shim: update annotation routes to use new platform router (elastic#57067) Remove injected ui app vars from Canvas (elastic#56190) update max_anomaly_score route schema to handle possible undefined values (elastic#57339) [Add panel flyout] Moving create new to the top of SavedObjectFinder (elastic#56428) Add mock of a legacy ui api to re-enable Canvas storybook (elastic#56673) [monitoring] Removes noisy event received log (elastic#57275) Remove use of copied MANAGEMENT_BREADCRUMBS and use `setBreadcrumbs` from management section's mount (elastic#57324) Advanced Settings management app to kibana platform plugin (elastic#56931) [ML] New Platform server shim: update recognize modules routes to use new platform router (elastic#57206) [ML] Fix overall stats for saved search on the Data Visualizer page (elastic#57312) [ML] [NP] Removing ui imports (elastic#56358) [SIEM] Fixes failing Cypress tests (elastic#57202) Create observability CODEOWNERS reference (elastic#57109) fix results service schema (elastic#57217) don't register a wrapper if browser side function exists. (elastic#57196) Ui Actions explorer example (elastic#57006) Fix update alert API to still work when AAD is out of sync (elastic#57039) ...

darnautov added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 10, 2020

darnautov requested review from alvarezmelissa87 and peteharverson February 10, 2020 15:23

darnautov requested a review from a team as a code owner February 10, 2020 15:23

darnautov self-assigned this Feb 10, 2020

alvarezmelissa87 reviewed Feb 10, 2020

View reviewed changes

x-pack/legacy/plugins/ml/server/models/data_recognizer/data_recognizer.ts Outdated

Copy link
Copy Markdown

Contributor

alvarezmelissa87 Feb 10, 2020

Choose a reason for hiding this comment

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

Same here

jgowdyelastic reviewed Feb 10, 2020

View reviewed changes

jgowdyelastic reviewed Feb 11, 2020

View reviewed changes

darnautov added 4 commits February 11, 2020 10:54

[ML] modules with NP router

c730ba8

[ML] use extendedRouteInitializationDeps

fb7b555

[ML] add apidoc annotations

9ca99ad

[ML] address PR comments

63ea75b

darnautov force-pushed the ML-49743-modules branch from 1f975d4 to 63ea75b Compare February 11, 2020 10:00

darnautov requested review from alvarezmelissa87 and jgowdyelastic February 11, 2020 10:05

[ML] convert data_recognizer test to jest

9edbdec

jgowdyelastic approved these changes Feb 11, 2020

View reviewed changes

peteharverson mentioned this pull request Feb 11, 2020

[ML] New platform migration #49743

Closed

78 tasks

alvarezmelissa87 approved these changes Feb 11, 2020

View reviewed changes

[ML] optional indexPatternName

6d23884

darnautov merged commit b133357 into elastic:master Feb 11, 2020

darnautov deleted the ML-49743-modules branch February 11, 2020 16:11

darnautov mentioned this pull request Feb 11, 2020

[7.x] [ML] New Platform server shim: update recognize modules routes to use new platform router (#57206) #57335

Merged

Conversation

darnautov commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Feb 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 11, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

darnautov commented Feb 10, 2020 •

edited

Loading

alvarezmelissa87 Feb 10, 2020 •

edited

Loading

alvarezmelissa87 Feb 10, 2020 •

edited

Loading