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

Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 #56408

Conversation

kertal
Copy link
Member

@kertal kertal commented Jan 30, 2020

Summary

Migrate the following dependencies to kibana_legacy while keeping them compatible with the old platform by importing static functions from NP.

  • "Private" ui/private/private
  • Promises ui/promises/promises
  • migrateLegacyQuery ui/utils/migrate_legacy_query
  • ui/compat
  • ui/legacy_compat && parts of ui/notify
  • ui/directives/watch_multi/watch_multi
  • ui/accessibility/kbn_accessible_click
  • ui/directives/listen/listen

Part of #50670

Checklist

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

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] 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 accessibility

For 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

@kertal kertal self-assigned this Jan 30, 2020
@kertal kertal changed the title Kibana app migration: Move static code dependencies into kibana_legacy plugin Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 Feb 4, 2020
@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 5, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@kertal kertal marked this pull request as ready for review February 5, 2020 12:11
@kertal kertal requested a review from a team February 5, 2020 12:11
@kertal kertal requested review from a team as code owners February 5, 2020 12:11
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in Spaces plugin LGTM.

@kertal kertal added the release_note:skip Skip the PR/issue when compiling release notes label Feb 5, 2020
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks mostly good, left some nits. Great how little stuff there's still left in the legacy imports

@@ -374,21 +383,21 @@ const $setupUrlOverflowHandling = (newPlatform: CoreStart, isLocalAngular: boole
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't have to happen in this PR, but we should probably pull the gist of these helpers out of the angular context and put them into generic helpers (e.g. for the ui capability redirect or the url overflow redirect)

uiModules.get('kibana', ['ngSanitize']).filter('markdown', function($sanitize) {
return md => (md ? $sanitize(markdownIt.render(md)) : '');
});
export * from './migrate_legacy_query';
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought - is migrate_legacy_query more of a data plugin responsibility? Not sure how to handle this. Can also be handled in a separate issue

Copy link
Member Author

Choose a reason for hiding this comment

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

about migrate_legacy_query:
Creates a standardized query object from old queries that were either strings or pure ES query DSL

Dear @elastic/kibana-app-arch 🏹 what do you think, should it be migrated to data

@@ -41,33 +41,31 @@ export { PersistedState } from 'ui/persisted_state';

export { npSetup, npStart } from 'ui/new_platform';
export { IPrivate } from 'ui/private';
Copy link
Contributor

Choose a reason for hiding this comment

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

The code for private is moved, lets also move the type and reference it via re-export.

Copy link
Member Author

Choose a reason for hiding this comment

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

thx! will clean up!

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

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

Canvas change is good 👍

@kertal kertal requested a review from flash1293 February 6, 2020 14:58
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the cleanup 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@elastic elastic deleted a comment from kibanamachine Feb 7, 2020
@kertal kertal merged commit c4fe6ec into elastic:master Feb 7, 2020
kertal added a commit to kertal/kibana that referenced this pull request Feb 7, 2020
…y plugin, part 1 (elastic#56408)

* Migrate PromiseServiceCreator

* Migrate migrateLegacyQuery

* Migrate watch_multi decorator

* Migrate call_after_bindings_workaround

* Migrate configureAppAngularModule, ensureDefaultIndexPattern and deps

* Migrate kbn_accessible_clicks

* Migrate PrivateProvider

* Migrate registerListenEventListener

* Adapt i18n files
kertal added a commit that referenced this pull request Feb 7, 2020
…y plugin, part 1 (#56408) (#57095)

* Migrate PromiseServiceCreator

* Migrate migrateLegacyQuery

* Migrate watch_multi decorator

* Migrate call_after_bindings_workaround

* Migrate configureAppAngularModule, ensureDefaultIndexPattern and deps

* Migrate kbn_accessible_clicks

* Migrate PrivateProvider

* Migrate registerListenEventListener

* Adapt i18n files
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 7, 2020
…b.com:jloleysens/kibana into console/feature/text-objects-in-saved-objects

* 'console/feature/text-objects-in-saved-objects' of github.com:jloleysens/kibana: (103 commits)
  fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998)
  TS of esKuery\node_types  (elastic#56857)
  Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 (elastic#56408)
  Retry ES API calls that fail with 410/Gone (elastic#56950)
  [APM] Show missing permissions message to the user on the Services overview (elastic#56374)
  Fixing flaky CI tests for custom appRoutes (elastic#55763)
  [State Management][Docs] State syncing utils docs (elastic#56479)
  [Index management] Remove index mapper setting in tests (elastic#57066)
  Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types  (elastic#56466)
  [SIEM] update url state between page if date is relative (elastic#56813)
  fix for chart_types test (elastic#57056)
  chore(NA): remove compress from dll minimizer (elastic#57023)
  [File upload] Migrate routing to NP & add route validation (elastic#52313)
  Adding docs for grouped nav advanced setting (elastic#57013)
  Use i18n titles for field formatters, human names for numeral locales (elastic#56348)
  [Maps] Remove EMS catalogue url from docs (elastic#57020)
  [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (elastic#56538)
  [DOCS] Adds Apple notarization info to install doc (elastic#57042)
  [ML] New Platform server shim: update results service routes to use new platform router (elastic#56886)
  Fix typo on detection engine rule (elastic#56993)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants