[ML] New Platform Migration - server #38360
Conversation
alvarezmelissa87
requested review from
epixa,
walterra,
peteharverson and
jgowdyelastic
|
Pinging @elastic/ml-ui |
💔 Build Failed |
alvarezmelissa87
force-pushed
the
ml-server-migration-step-one
branch
from
a5408d7 to
1da3526
Compare
💔 Build Failed |
💚 Build Succeeded |
alvarezmelissa87
changed the title
There was a problem hiding this comment.
The shimming/new-platform related changes in here look good. The important things - decoupling business logic from hapi and auditing dependencies on core and other plugins - are done in this PR.
The next migration steps for server-side stuff will be to start converting MlCoreSetup and PluginsSetup over to implementations provided by the new platform (not all of which are yet available at the time of this writing).
Great job!
| export const REQUIRED_ROLES = ['machine_learning_admin', 'machine_learning_user']; | ||
| export const REQUIRED_LICENSES = ['standard', 'gold', 'trial', 'platinum']; | ||
| export const LICENSES = ['oss', 'basic', 'standard', 'gold', 'trial', 'platinum']; | ||
| export type LicenseType = 'oss' | 'basic' | 'trial' | 'standard' | 'basic' | 'gold' | 'platinum'; |
There was a problem hiding this comment.
Is there a way of defining these license type values just once, for example in an enum, rather than using the Strings in multiple definitions?
| elasticsearch: ElasticsearchPlugin; | ||
| xpackMain: MlXpackMainPlugin; | ||
| security: any; | ||
| // TODO: this is temporary for `mirrorPluginStatus` |
There was a problem hiding this comment.
Just to confirm, is this still a TODO?
There was a problem hiding this comment.
This is a TODO in the sense that in the legacy init function we were passing the instance of the plugin itself to the mirrorPluginStatus function. I wanted to note that that may change as the new platform services become available. It might not - I just wanted to remind myself to check as we move forward.
alvarezmelissa87
added
the
release_note:skip
| log: Logger; | ||
| } | ||
| export class Plugin { | ||
| private readonly pluginId: string = 'ml'; |
There was a problem hiding this comment.
this could probably be moved out of the class as a PLUGIN_ID
| }, | ||
| mappings, | ||
| home: ['plugins/ml/register_feature'], | ||
| injectDefaultVars(server: any) { |
There was a problem hiding this comment.
could this be a Server or MlServer?
If not, does kibana have any types for this? also the kibana parameter in the outer function.
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| export const REQUIRED_ROLES = ['machine_learning_admin', 'machine_learning_user']; |
There was a problem hiding this comment.
These three arrays aren't used, what are they for?
There are also data frame roles which might be required. If these are going to be used to grant access to something, should we instead be relying on the underlying privileges rather than the roles? As new roles which are identical to these could be created and so should grant the same access.
alvarezmelissa87
force-pushed
the
ml-server-migration-step-one
branch
from
efa0019 to
709a32f
Compare
💔 Build Failed |
💚 Build Succeeded |
* wip: pull out all server dependencies * create new platform plugin * shim newPlatform plugin into legacy init * update server tests for migration refactor * cleanup interfaces * Only add ML links for sample data sets if full license from - 38120 * update test and fix typescript errors * Remove unused types
Summary
This is the first phase of the ML plugin migration to the new platform - this is only addressing the server side implementation.
The new platform definition lives in
server/new_platform.The legacy plugin definition is still the one being executed, so this PR shims the new plugin definition into the still-used legacy one by instantiating it and wiring it up inside the init function.
Followups
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Documentation was added for features that require explanation or tutorials- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately