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] [NP] Removing ui imports #56358

Merged
merged 30 commits into from
Feb 11, 2020

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jan 30, 2020

Removed all imports from 'ui/...'
Makes use of the kibana context where possible. For utility functions which are non-react, a cache of imported dependencies has been added. These are cleared when the app is closed using onAppLeave
Removes the use of injectI18n for translations.
Uses withKibana for components which can't use useMlKibana hook.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@jgowdyelastic jgowdyelastic force-pushed the ml-np-removing-ui-imports branch 2 times, most recently from 9682027 to b47cee3 Compare February 3, 2020 10:59
@jgowdyelastic
Copy link
Member Author

retest

@jgowdyelastic jgowdyelastic force-pushed the ml-np-removing-ui-imports branch from a44e363 to 4088397 Compare February 6, 2020 12:17
@jgowdyelastic jgowdyelastic marked this pull request as ready for review February 6, 2020 19:35
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner February 6, 2020 19:35
@jgowdyelastic jgowdyelastic self-assigned this Feb 6, 2020
@jgowdyelastic jgowdyelastic added :ml Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v8.0.0 labels Feb 6, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

💔 Build Failed

History

  • 💔 Build #24887 failed 4088397e377a4fe4c1c74a5d5e5dd96bd32a26d5
  • 💔 Build #24876 failed a44e3639f8be4e01e40a570b15613b3ad7787682
  • 💚 Build #24344 succeeded 7963cd9665a8dd8e5e318f0a0e540bffa1201ada
  • 💚 Build #24298 succeeded e2f96a12055960c7b5ef80f50423863c3b0f041a
  • 💔 Build #24274 failed 20748f95d6ad43fe256df91e4a7cd34bca59e87b

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

@elastic elastic deleted a comment from kibanamachine Feb 6, 2020
@jgowdyelastic jgowdyelastic force-pushed the ml-np-removing-ui-imports branch from a05ea62 to 286d559 Compare February 6, 2020 20:50
@elasticmachine
Copy link
Contributor

💔 Build Failed

History

  • 💔 Build #25011 failed 0d1500ef621dfb5d7555f4aa70bc8e3149fac051
  • 💔 Build #24887 failed 4088397e377a4fe4c1c74a5d5e5dd96bd32a26d5
  • 💔 Build #24876 failed a44e3639f8be4e01e40a570b15613b3ad7787682
  • 💚 Build #24344 succeeded 7963cd9665a8dd8e5e318f0a0e540bffa1201ada
  • 💚 Build #24298 succeeded e2f96a12055960c7b5ef80f50423863c3b0f041a

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #25036 failed b8f61d62b1c140a3f4cdea22f97a2bbbee51cd70
  • 💔 Build #25011 failed 0d1500ef621dfb5d7555f4aa70bc8e3149fac051
  • 💔 Build #24887 failed 4088397e377a4fe4c1c74a5d5e5dd96bd32a26d5
  • 💔 Build #24876 failed a44e3639f8be4e01e40a570b15613b3ad7787682
  • 💚 Build #24344 succeeded 7963cd9665a8dd8e5e318f0a0e540bffa1201ada

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #25075 succeeded d36fda38721403d35f48d47d5e9c73a9c6aafe9a
  • 💔 Build #25036 failed b8f61d62b1c140a3f4cdea22f97a2bbbee51cd70
  • 💔 Build #25011 failed 0d1500ef621dfb5d7555f4aa70bc8e3149fac051
  • 💔 Build #24887 failed 4088397e377a4fe4c1c74a5d5e5dd96bd32a26d5
  • 💔 Build #24876 failed a44e3639f8be4e01e40a570b15613b3ad7787682

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

defaultMessage: 'Forecasts',
}),
content: <ForecastsTable job={job} />,
}
);
}

if (mlAnnotationsEnabled && showFullDetails) {
if (showFullDetails) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the mlAnnotationsEnabled check not needed any more?

Copy link
Member Author

@jgowdyelastic jgowdyelastic Feb 7, 2020

Choose a reason for hiding this comment

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

as part of this work i removed the mlAnnotationsEnabled flag.
it was originally being set on the server from a constant value that never changed from being true.
it was then being imported using a chrome function that needed to be removed.
I discussed with @walterra and we agreed that it was redundant code that could be removed.

import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';

export class DeleteJobModal extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this console warning when deleting a job. Is it related to your changes?

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I see this on master also. So not a new warning

@jgowdyelastic jgowdyelastic force-pushed the ml-np-removing-ui-imports branch 2 times, most recently from 76e7a27 to 70b18a8 Compare February 9, 2020 21:14
@peteharverson
Copy link
Contributor

If I try and access the Jobs List from the Management page, I see this error, and the jobs list fails to display:

image

@jgowdyelastic jgowdyelastic force-pushed the ml-np-removing-ui-imports branch from 36dd3eb to 8464f2a Compare February 11, 2020 13:00
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jgowdyelastic jgowdyelastic merged commit b3e8bdf into elastic:master Feb 11, 2020
@jgowdyelastic jgowdyelastic deleted the ml-np-removing-ui-imports branch February 11, 2020 16:19
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Feb 11, 2020
* [ML] [NP] Removing ui imports

* replacing timefilter and ui context

* replacing ui/i18n and ui/metadata

* removing ui/system_api

* removing ui/notify

* removing most ui/new_platform

* fix explorer date format loading

* removing ui/chrome

* fixing timebuckets test

* fixing jest tests

* adding http

* testing odd CI type failure

* revrting type test changes

* fixing odd test type error

* refactoring dependencies

* removing injectI18n and using withKibana for context

* updating components to use kibana context

* re-enabling some tests

* fixing translation strings

* adding comments

* removing commented out code

* missing i18n

* fixing rebase conflicts

* removing unused ui contexts

* changes based on review

* adding text to errors

* fixing management plugin

* changes based on review

* refeactor after rebase

* fixing test
jgowdyelastic added a commit that referenced this pull request Feb 11, 2020
* [ML] [NP] Removing ui imports

* replacing timefilter and ui context

* replacing ui/i18n and ui/metadata

* removing ui/system_api

* removing ui/notify

* removing most ui/new_platform

* fix explorer date format loading

* removing ui/chrome

* fixing timebuckets test

* fixing jest tests

* adding http

* testing odd CI type failure

* revrting type test changes

* fixing odd test type error

* refactoring dependencies

* removing injectI18n and using withKibana for context

* updating components to use kibana context

* re-enabling some tests

* fixing translation strings

* adding comments

* removing commented out code

* missing i18n

* fixing rebase conflicts

* removing unused ui contexts

* changes based on review

* adding text to errors

* fixing management plugin

* changes based on review

* refeactor after rebase

* fixing test
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 11, 2020
* 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)
  ...
@alexwizp alexwizp mentioned this pull request Feb 12, 2020
7 tasks
walterra added a commit that referenced this pull request Feb 18, 2020
Fixes a regression introduced by #56358. We're reusing the KqlFilterBar from the ML plugin in the transform plugin and missed updating the dependency management. Note this PR is only about fixing that regression. Future work on NP migration should take care of cleaning up the dependency management in the transforms plugin in general.
walterra added a commit to walterra/kibana that referenced this pull request Feb 18, 2020
Fixes a regression introduced by elastic#56358. We're reusing the KqlFilterBar from the ML plugin in the transform plugin and missed updating the dependency management. Note this PR is only about fixing that regression. Future work on NP migration should take care of cleaning up the dependency management in the transforms plugin in general.
walterra added a commit that referenced this pull request Feb 18, 2020
…7852)

Fixes a regression introduced by #56358. We're reusing the KqlFilterBar from the ML plugin in the transform plugin and missed updating the dependency management. Note this PR is only about fixing that regression. Future work on NP migration should take care of cleaning up the dependency management in the transforms plugin in general.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration :ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants