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

Migrate home app #58030

Merged
merged 20 commits into from
Mar 4, 2020
Merged

Migrate home app #58030

merged 20 commits into from
Mar 4, 2020

Conversation

flash1293
Copy link
Contributor

This PR moves the home app over into the new platform. The only thing being left behind are static resources which are currently not supported by the new platform, those will be moved in a separate PR once the core api is available.

This PR also removes some exposed apis which were only necessary to pass information from the NP home plugin to the legacy home plugin.

@flash1293 flash1293 added Feature:Home Kibana home application v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 19, 2020
@flash1293 flash1293 marked this pull request as ready for review February 21, 2020 17:57
@flash1293 flash1293 requested a review from a team February 21, 2020 17:57
@flash1293 flash1293 requested review from a team as code owners February 21, 2020 17:57
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Feb 21, 2020
@elasticmachine
Copy link
Contributor

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

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Feb 24, 2020

just started to test it, seems there's a problem when you click on "Add Apm"
Bildschirmfoto 2020-02-24 um 11 41 28

Same error with other tutorials like
http://localhost:5601/aft/app/kibana#/home/tutorial/apacheLogs

@flash1293
Copy link
Contributor Author

Thanks for checking @kertal , seems like there are some imports of. I will check and fix 👍

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

infra changes lgtm

@flash1293 flash1293 requested a review from a team as a code owner March 2, 2020 09:48
// Visualize styles
@import './visualize/index';
// Has to come after visualize because of some
// bad cascading in the Editor layout
@import 'src/legacy/ui/public/vis/index';

// Home styles
@import '../../../../plugins/home/public/application/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you import this file directly in the JS in plugins/home/public/ instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

And also then rename it from _index.scss to index.scss

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 originally included this but reverted because the scss is depending on legacy mixins in “ui/styles”. Importing from a module in the new platform gave me an error. The platform team is owning the topic and is tracking this separately, as soon as it’s moved the entry-point can be moved over just as you suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could then just import the mixin dependencies directly in the index.scss file so as to unblock moving this to the JS file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s not complaining about the mixin itself (somehow it’s pulling that in implicitly), but about an asset used within the mixin: https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/styles/_mixins.scss#L92

That’s the line failing. As it’s going to get migrated soon I didn’t dig any deeper , but maybe you know a better way around the problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does the error say?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I wonder then why yours would be the only implementation that complains if it's not the one pulling the file directly...

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 couldn't reproduce the problem locally anymore and the Jenkins build with this failure is already cleaned up - I committed the change again, if it works even better.

Also, I wonder then why yours would be the only implementation that complains if it's not the one pulling the file directly...

There is no usage of that specific mixin from within the new platform so far, it's only consumed the legacy way:

x-pack/plugins/spaces/public/space_selector/_space_selector.scss
2:  @include kibanaFullScreenGraphics;

x-pack/legacy/plugins/security/public/components/authentication_state_page/_authentication_state_page.scss
2:  @include kibanaFullScreenGraphics;

x-pack/legacy/plugins/security/public/views/login/components/login_page/_login_page.scss
2:  @include kibanaFullScreenGraphics;

src/plugins/home/public/application/components/_welcome.scss
2:  @include kibanaFullScreenGraphics($euiZLevel6);

Spaces selector looks like it's imported in the new platform, but it's still used in the old way eventually: x-pack/legacy/plugins/spaces/public/index.scss

Copy link
Contributor

Choose a reason for hiding this comment

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

I just pulled down your branch, ran it locally, didn't get any compile or console errors and the graphics rendered properly in the Welcome page. So... 👍 looks like all is good with the import in JS?

@flash1293
Copy link
Contributor Author

@kertal This PR should be ready to be checked again from the JS side of things - fixed the broken tutorials.

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, waving goodbye to home from legacy world, may the static assets follow soon.
No more JS problems detected, tested locally in chrome!

@flash1293
Copy link
Contributor Author

flash1293 commented Mar 3, 2020

@cchaos That’s the build failure I was referring to:
Module not found: Error: Can't resolve './ui/assets/images/bg_bottom_branded.svg' in '/dev/shm/workspace/kibana/build/kibana-oss/src/plugins/home/public/application'

Do you have an idea what this is about?

@@ -21,13 +21,9 @@ import { PluginInitializerContext } from 'kibana/public';

export {
FeatureCatalogueSetup,
Copy link
Contributor

@lizozom lizozom Mar 3, 2020

Choose a reason for hiding this comment

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

If you're exposing only the setup contract of these sub-services, maybe expose them as FeatureCatalogueSetup as FeatureCatalogue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will fix that

this.capabilities = capabilities;
}

public get(): readonly FeatureCatalogueEntry[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was exposed because the home app was still part of the legacy platform while the registry already moved. Now both are back together in a single plugin and there is no need anymore to expose that information.

this.capabilities = capabilities;
}

public get(): readonly FeatureCatalogueEntry[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was exposed because the home app was still part of the legacy platform while the registry already moved. Now both are back together in a single plugin and there is no need anymore to expose that information.

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.

Added one minor comment

@flash1293
Copy link
Contributor Author

@cchaos as discussed offline, I reverted the new platform scss import for now.

@flash1293
Copy link
Contributor Author

Jenkins, test this.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@flash1293 flash1293 merged commit cba4f34 into elastic:master Mar 4, 2020
flash1293 added a commit to flash1293/kibana that referenced this pull request Mar 4, 2020
flash1293 added a commit that referenced this pull request Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Home Kibana home application 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.

7 participants