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: Shim dashboard #48913

Merged
merged 88 commits into from
Nov 20, 2019
Merged

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Oct 22, 2019

Shims dashboard app and uses local angular instance instead of references to global angular to be able to move this to the new platform without getting rid of angular first.

There are quite some changes outside of the dashboard app itself in here, so I'll try to put together a guide for how to review this:

  • Data plugin (AppArch): filter bar directives were directly registered to uiModules which we can't use in our local angular. This PR exports the raw direcrtive definitions so they can get registered in a local angular module as well.
  • Discover (KibanaApp): requireDefaultIndex magic flag on the route got removed because this requires uiRoutes we won't carry over (not used in many places anyway). The replacement is a function that has to be called in the only resolve of a route, if the returned promise resolves the actual data can be fetched. That slightly changes the code structure in discover because it was fetching the index pattern and the saved search in two separate resolve props.
  • Management (AppArch): The logic redirecting to either home or the index_pattern page when there is no index pattern got moved into the requireDefaultIndex helper function.
  • Visualize editor (KibanaApp): Same as for discover
  • ui/capabilities/route_setup (KibanaApp/AppArch?): Got moved to src/legacy/ui/public/legacy_compat/angular_config.tsx because it's currently also using the addSetupWork hook.
  • legacy_compat angular stuff (Platform?):
    • configureAppAngularModule was using NP dependencies directly, to be able to use the same function also in the new platform this was changed to take them via parameter - also be adjusted in legacy chrome src/legacy/ui/public/chrome/api/angular.js
    • Fixes a bug in the dummy wrapper route detection of local angular that resets title in some cases when moving from a dummy wrapper route to an actual angular controlled route.
    • Move ui-capability redirects here from the removed setup work
  • TopNav (AppArch): Same as for the filter bar directives - exposing the directive definitions directly allows to have them registered in a local angular module just for dashboard.
  • State mangement (AppArch?): Make sure the state instances only follow along but do not interfere with the URL as long as the user is within the local angular controlled prefix.
  • src/legacy/ui/public/timefilter/setup_router.ts (AppArch?): Expose the logic to wire the global state instance with the NP data plugin timefilter state so it's usable in the local (and make sure it's also unsubscribing correctly)
  • Graph: Pass new navigation dep through to the angular directive creation.

Dev docs

The route flag requireDefaultIndex making sure there are index patterns and the defaultIndex advanced setting is set was removed.

The same functionality can be achieved by using the helper function ensureDefaultIndexPattern from ui/legacy_compat within the resolve object of a route.

@flash1293 flash1293 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.6.0 labels Oct 22, 2019
Copy link
Member

@kertal kertal 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 👍 , tested locally comparing side by side the non-shimmed version, and since I merged it into my Discover PR, this was also tested this way. Just one note, the icon directive seems to be missing

image

Just add .directive('icon', reactDirective => reactDirective(EuiIcon)) to your inner angular directive to make it work

@flash1293 flash1293 requested a review from lizozom November 18, 2019 15:30
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

In terms of @elastic/kibana-app-arch services, LGTM
I will follow up with a PR deprecating filter-bar directives.

@flash1293
Copy link
Contributor Author

Good catch @kertal !

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

i think you shouldn't be exporting any legacy angular stuff from np_ready plugins. we plan to remove that soon, and if you depend on any of those items (as you plan to keep angular past 8.0) you should copy the relevant code inside your plugin.

@flash1293
Copy link
Contributor Author

@ppisljar PR #50661 will create a kibana_legacy plugin in the new platform that's planned to house these things. Also see #50670

So instead of copying those code snippets into each separate plugin, we plan to just move them into this plugin. I just didn't do this as part of this PR because it is touching enough places already.

@flash1293 flash1293 requested a review from ppisljar November 19, 2019 13:21
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor Author

Jenkins, test this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 merged commit 9cb37a1 into elastic:master Nov 20, 2019
@flash1293 flash1293 deleted the shim-dashboard branch November 20, 2019 08:44
@kertal
Copy link
Member

kertal commented Nov 20, 2019

🎉 !!!!

flash1293 added a commit to flash1293/kibana that referenced this pull request Nov 20, 2019
TinaHeiligers pushed a commit that referenced this pull request Nov 20, 2019
* Kibana app migration: Shim dashboard (#48913)

* fix location of feature catalogue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants