[ML] Transforms: NP ui/imports migration #58469

walterra · 2020-02-25T13:19:09Z

Summary

Part of #50902.

Migrates ui/* imports to NP.
Uses direct React mounting instead of Angular.
Service Cleanup.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-25T13:19:11Z

Pinging @elastic/ml-ui (:ml)

sebelga

Great work! Code looks good to me. I really like how you import doc links through hooks instead of a documentation service! We'll get inspired by it in our apps 😊

sebelga · 2020-02-26T05:21:31Z

x-pack/legacy/plugins/transform/public/app/app.tsx

@@ -5,8 +5,9 @@
 */

 import React, { useContext, FC } from 'react';
+import { unmountComponentAtNode } from 'react-dom';


nit: you could add the import inside the line below

Fixed in 299f912.

sebelga · 2020-02-26T05:26:17Z

x-pack/legacy/plugins/transform/public/app/common/navigation.tsx

@@ -29,9 +29,9 @@ export function getDiscoverUrl(indexPatternId: string, baseUrl: string): string
 }

 export const RedirectToTransformManagement: FC = () => (
-  <Redirect from={`${CLIENT_BASE_PATH}`} to={`${CLIENT_BASE_PATH}/${SECTION_SLUG.HOME}`} />
+  <Redirect from={`${CLIENT_BASE_PATH}`} to={`${CLIENT_BASE_PATH}${SECTION_SLUG.HOME}`} />


nit: as there is not string character, it could simply be

from={CLIENT_BASE_PATH} to={CLIENT_BASE_PATH + SECTION_SLUG.HOME}

Fixed in 299f912.

sebelga · 2020-02-26T05:28:38Z

x-pack/legacy/plugins/transform/public/app/components/section_error.tsx

-    message,
-  } = error.data;
+  const errorMessage =
+    error?.message !== undefined ? error.message : JSON.stringify(error, null, 2);


nit: could be written error?.message ?? JSON.stringify(error, null, 2);

Fixed in 299f912.

sebelga · 2020-02-26T05:33:41Z

x-pack/legacy/plugins/transform/public/app/hooks/use_delete_transform.tsx

-        text: toMountPoint(<ToastNotificationText text={e} />),
+        text: toMountPoint(
+          <ToastNotificationText
+            text={e.message !== undefined ? e.message : JSON.stringify(e, null, 2)}


nit: could be written e.message ?? JSON.stringify(e, null, 2)

Fixed in 299f912.

walterra · 2020-02-26T08:19:50Z

@joshdover any hints how I could migrate away from chrome.getXsrfToken()?

jgowdyelastic · 2020-02-26T11:42:52Z

x-pack/legacy/plugins/transform/public/app/lib/kibana/kibana_provider.tsx

@@ -61,7 +58,7 @@ export const KibanaProvider: FC<Props> = ({ savedObjectId, children }) => {
    const kibanaContext: InitializedKibanaContextValue = {
      indexPatterns,
      initialized: true,
-      kbnBaseUrl: npStart.core.injectedMetadata.getBasePath(),
+      kbnBaseUrl: appDeps.core.injectedMetadata.getBasePath(),


basePath can be got from http, i believe injectedMetadata is deprecated.

Fixed in 5757826.

jgowdyelastic · 2020-02-26T11:52:14Z

x-pack/legacy/plugins/transform/public/shim.ts

@@ -124,6 +103,7 @@ export function createPublicShim(): { core: Core; plugins: Plugins } {
      uiMetric: {
        createUiStatsReporter,
      },
+      xsrfToken: chrome.getXsrfToken(),


ML will soon be replacing our http service with kibana's core http client to remove the need of this token.

peteharverson · 2020-02-26T15:11:40Z

x-pack/legacy/plugins/transform/public/app/app_dependencies.tsx

 let DependenciesContext: React.Context<AppDependencies>;

 const setAppDependencies = (deps: AppDependencies) => {
+  const legacyBasePath = {


Running Kibana with a base path, I am seeing this error:

and I see a 404 in the console which shows a doubling up of the base path kch
GET http://localhost:5601/kch/kch/api/transform/privileges 404 (Not Found)

Fixed in aa8535b.

kibanamachine · 2020-02-28T13:01:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5757826

History

💔 Build #29050 failed 35655d5
💔 Build #28902 failed 4fb6904
💚 Build #28802 succeeded 1f078cd

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

jgowdyelastic

Code LGTM

- Migrates ui/* imports to NP. - Uses direct React mounting instead of Angular. - Service Cleanup.

walterra added 4 commits February 25, 2020 11:15

[ML] Migrate toastNotifications.

3f744af

[ML] Simplifly refresh interval.

f89462e

[ML] Migrate documentation links.

d394773

[ML] Mirage ui imports. Update tests.

1f078cd

walterra added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Transforms ML transforms Feature:NP Migration v7.7.0 labels Feb 25, 2020

walterra self-assigned this Feb 25, 2020

walterra added 2 commits February 25, 2020 19:36

[ML] MOAR NP migration.

fe32903

[ML] http service cleanup.

4fb6904

walterra marked this pull request as ready for review February 25, 2020 18:51

walterra requested a review from a team as a code owner February 25, 2020 18:51

walterra requested review from darnautov, jgowdyelastic, peteharverson, alvarezmelissa87 and joshdover February 25, 2020 18:52

sebelga reviewed Feb 26, 2020

View reviewed changes

walterra added 2 commits February 26, 2020 09:09

[ML] Review feedback.

299f912

[ML] Fixes loading wizard with saved search.

35655d5

walterra mentioned this pull request Feb 26, 2020

[ML] Transforms: New platform migration #50902

Closed

39 tasks

jgowdyelastic reviewed Feb 26, 2020

View reviewed changes

peteharverson reviewed Feb 26, 2020

View reviewed changes

walterra requested a review from pgayvallet February 27, 2020 16:14

Merge branch 'master' into ml-tranforms-deprecate-ui-imports

4b44e35

walterra added 2 commits February 28, 2020 11:38

[ML] Fix unnecessary base path.

aa8535b

[ML] Deprecate kbnBaseUrl.

5757826

jgowdyelastic approved these changes Feb 28, 2020

View reviewed changes

walterra merged commit 8f9004b into elastic:master Feb 28, 2020

walterra deleted the ml-tranforms-deprecate-ui-imports branch February 28, 2020 13:28

walterra mentioned this pull request Feb 28, 2020

[7.x] [ML] Transforms: NP ui/imports migration (#58469) #58868

Merged

walterra added a commit to walterra/kibana that referenced this pull request Feb 28, 2020

[ML] Transforms: NP ui/imports migration (elastic#58469)

74c91be

- Migrates ui/* imports to NP. - Uses direct React mounting instead of Angular. - Service Cleanup.

walterra added a commit that referenced this pull request Feb 28, 2020

[ML] Transforms: NP ui/imports migration (#58469) (#58868)

e67873f

- Migrates ui/* imports to NP. - Uses direct React mounting instead of Angular. - Service Cleanup.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Transforms: NP ui/imports migration #58469

[ML] Transforms: NP ui/imports migration #58469

walterra commented Feb 25, 2020 •

edited

Loading

elasticmachine commented Feb 25, 2020

sebelga left a comment

sebelga Feb 26, 2020

walterra Feb 26, 2020

sebelga Feb 26, 2020

walterra Feb 26, 2020

sebelga Feb 26, 2020

walterra Feb 26, 2020

sebelga Feb 26, 2020

walterra Feb 26, 2020

walterra commented Feb 26, 2020

jgowdyelastic Feb 26, 2020

walterra Feb 28, 2020

jgowdyelastic Feb 26, 2020

peteharverson Feb 26, 2020

walterra Feb 28, 2020

kibanamachine commented Feb 28, 2020

jgowdyelastic left a comment

[ML] Transforms: NP ui/imports migration #58469

[ML] Transforms: NP ui/imports migration #58469

Conversation

walterra commented Feb 25, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 25, 2020

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra commented Feb 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Feb 28, 2020

💚 Build Succeeded

History

jgowdyelastic left a comment

Choose a reason for hiding this comment

walterra commented Feb 25, 2020 •

edited

Loading