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] Register NP ML plugin for Kibana management section. #59762

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

walterra
Copy link
Contributor

@walterra walterra commented Mar 10, 2020

Summary

Migrates registering the ML management section using React only, deprecates the angularJS approach.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@walterra walterra self-assigned this Mar 10, 2020
@walterra
Copy link
Contributor Author

@joshdover I'm working on this PR which migrates the ML section in Kibana management to use the NP services. Unfortunately I'm not able to successfully register the section+menu item, it's not showing up. Any feedback very much appreciated.

@walterra
Copy link
Contributor Author

@joshdover just a ping to let you know i was able to solve it! revisiting with a fresh mind helped :)

@walterra walterra marked this pull request as ready for review March 11, 2020 12:19
@walterra walterra requested a review from a team as a code owner March 11, 2020 12:19
@walterra walterra added :ml Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features Feature:NP Migration refactoring release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@@ -10,22 +10,27 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { npSetup } from 'ui/new_platform';
import { management } from 'ui/management';
import { npSetup, npStart } from 'ui/new_platform';
Copy link
Member

@jgowdyelastic jgowdyelastic Mar 11, 2020

Choose a reason for hiding this comment

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

i don't think we should be importing anything from ui/new_platform anymore.
I wonder if this whole file needs to go and for the management section registration to happen in our root plugin.ts file? or at least get called from there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this can be done as part of this PR. The only reference that ties into this code is managementSections: ['plugins/ml/application/management'] in plugins/ml/index.ts and I can't see how I'd pass on something to here. If we can think of a solution I'm happy to do it in this PR, however, the original scope here was only to migrate away from angular :). For transforms, the "cut-over" PR was not just moving files but it also included updating the plugins/transfrom/index.ts file to work again after the move, maybe we need to do it this way.

@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.

Code 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 ⚡️

@walterra walterra merged commit 29ed1b7 into elastic:master Mar 12, 2020
@walterra walterra deleted the ml-management-np branch March 12, 2020 08:36
walterra added a commit that referenced this pull request Mar 12, 2020
…59985)

Migrates registering the ML management section using React only, deprecates the angularJS approach.
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 12, 2020
* master: (45 commits)
  skip flaky suite (elastic#59717)
  UI Metrics use findAll to retrieve all Saved Objects (elastic#59891)
  [Discover] Migrate Context mocha tests to use Jest (elastic#59658)
  [Maps] Move redux reducers and store logic to NP (elastic#58294)
  rebalance x-pack groups (elastic#58930)
  [Discover] Reimplement $route.reload when index pattern changes (elastic#59877)
  [Upgrade Assistant Meta] Breaking changes issue template (elastic#59745)
  Skip CI based on changes in PR (elastic#59939)
  [ML] Transforms: Replace KqlFilterBar with QueryStringInput. (elastic#59723)
  [ML] Functional tests - stabilize date_nanos test (elastic#59986)
  [ML] Typescripting client side endpoint functions (elastic#59928)
  a11y tests on adding columns to discover table (elastic#59375)
  fix graph plugin config path (elastic#59540)
  fix vega config issues (elastic#59737)
  [Upgrade Assistant] Open And Close Slight Refactor (elastic#59890)
  [ML] Adding shared services to ml setup contract (elastic#59730)
  [Visualize] Fix linked search behavior (elastic#59690)
  [ML] Register NP ML plugin for Kibana management section. (elastic#59762)
  [Lens] Adds using queries/filters for field existence endpoint (elastic#59033)
  Delete FilterStateManager and QueryFilter :-D (elastic#59872)
  ...
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Mar 12, 2020
)

Migrates registering the ML management section using React only, deprecates the angularJS approach.
@pgayvallet
Copy link
Contributor

just a ping to let you know i was able to solve it! revisiting with a fresh mind helped :)

@walterra To be able to help other teams that might encounter the same issue as you did, what was the problem and how did you fix it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:Data Frame Analytics ML data frame analytics features Feature:NP Migration :ml refactoring 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.

None yet

6 participants