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

[NP Kibana Migrations ] kibana plugin home #50444

Merged
merged 29 commits into from
Nov 18, 2019

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Nov 12, 2019

Summary

Refers to #50224
Creates a new plugin on the Kibana Platform for Home that will contain:

  1. Tutorials API and registration
  2. Features Catalogue
    (and potentially)
  3. sample data as well.

This PR only handles the Tutorial API and registration.

TODO:

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

For maintainers

Dev docs

The api to register new tutorials was moved to the new platform. You are now able to add new tutorials by creating a plugin in the new platform, adding a dependency to home in its kibana.json and using the tutorials.registerTutorial method in the setup lifecycle:

class MyPlugin {
  setup(core: CoreSetup, plugins: { home: HomeServerPluginSetup }) {
    home.tutorials.registerTutorial(() => ({ /* tutorial definition */ }));
  }
}

It is still possible to register tutorials from within the legacy platform by calling the same method exposed on the server object:

server.newPlatform.setup.plugins.home.tutorials.registerTutorial(() => ({ /* tutorial definition */ }));

@TinaHeiligers
Copy link
Contributor Author

@flash1293 What labels do we add to NP migration PR'S?

@flash1293
Copy link
Contributor

@TinaHeiligers Feature: NP Migration

@TinaHeiligers
Copy link
Contributor Author

@legrego @kobelb, we're in the process of migrating the tutorials registry api to the new platform and was hoping you'd be able to help with availability of getSpaceId in the New Platform on the request context:
From the original request in the issue:

@TinaHeiligers
Copy link
Contributor Author

@aaronjcaldwell could you help with the following blocker?

  • The EMS tutorial (src/legacy/core_plugins/kibana/server/tutorials/ems/index.js) is relying on the global config object to access server.basePath and map.emsLandingPageUrl. The basePath can be accessed via core.http.basePath.get but the map config will need to be provided by the Maps plugin.

@TinaHeiligers
Copy link
Contributor Author

@ogupte is this something you could help with the following blocker?

  • The APM tutorial (src/legacy/core_plugins/kibana/server/tutorials/apm/index.js) is relying on the legacy global config object to get access to the apm_oss.indexPattern and xpack.apm.ui.enabled keys. This will need to be refactored to get these configs directly from the apm_oss and apm plugins' setup contract.

@TinaHeiligers TinaHeiligers marked this pull request as ready for review November 14, 2019 23:38
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner November 14, 2019 23:38
@TinaHeiligers TinaHeiligers added Feature:NP Migration Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 14, 2019
@TinaHeiligers
Copy link
Contributor Author

The spaces changes look good. Regarding Joe's comment above:

The other two problems we can tackle later in a subsequent PR - if everything is using the new API we can remove the legacy mixin and also remove the legacy tutorial context factory call out of the spaces plugin.

Are you aware of anything else using the legacy context factory? I think it was originally introduced solely for the Spaces plugin, so we might be ready to remove it altogether? What do you think?

I don't think the legacy context factory is being used elsewhere but would prefer to verify this with the @elastic/kibana-platform team.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

I will let @eliperelman properly review, but it's looking good for me!

A few minor points and questions:

src/plugins/home/server/plugin.ts Outdated Show resolved Hide resolved
Comment on lines +34 to +42
const dashboardSchema = Joi.object({
id: Joi.string().required(), // Dashboard saved object id
linkLabel: Joi.string().when('isOverview', {
is: true,
then: Joi.required(),
}),
// Is this an Overview / Entry Point dashboard?
isOverview: Joi.boolean().required(),
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to be using joi directly instead of @kbn/config-schema?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Nov 16, 2019

Choose a reason for hiding this comment

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

That's the way it was prior to the move and I didn't want to introduce too many changes at once.
I did start but then ran into validations that aren't obviously available in @kbn/config-schema.
e.g. How do I go about validating an empty object for

query: {
  match_all: {},
},

query: {
  bool: {
    filter: {
      term: {
        'agent.type': 'auditbeat',
      },
    },
  },
},
query: {
  bool: {
    filter: [
      { term: { 'processor.event': 'onboarding' } },
      { range: { 'observer.version_major': { gte: 7 } } },
    ],
  },
},

WIth Joi validation, this is given as:

query: joi.object(),

I'll be happy to continue this effort with some assistance ;-) There's a throw-away, draft PR linked: TinaHeiligers#11

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the answer. Not everything in joi is available in config-schema, so migration may be not that obvious. Will try to look at the migration PR. joi is fine for now.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

…s api http routes for legacy platform and NP
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for me. Ideally would still like a second opinion from other assigned platform reviewers.

Comment on lines 24 to 25
const baseUrl = getServices().addBasePath('/api/kibana/home/tutorials_LP');
const baseUrlNP = getServices().addBasePath('/api/kibana/home/tutorials');
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: I would also rename the consts accordingly but this is very minor

server.newPlatform.setup.plugins.home.tutorials.registerTutorial(iptablesLogsSpecProvider);
server.newPlatform.setup.plugins.home.tutorials.registerTutorial(ciscoLogsSpecProvider);
server.newPlatform.setup.plugins.home.tutorials.registerTutorial(envoyproxyLogsSpecProvider);
server.newPlatform.setup.plugins.home.tutorials.registerTutorial(couchdbMetricsSpecProvider);
server.registerTutorial(emsBoundariesSpecProvider);
Copy link
Contributor

Choose a reason for hiding this comment

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

So apm and ems tutorials are the only ones still relying on legacy things and can't currently be migrated? Is this in the scope of the PR or will this be done in a next step when the plugins are ready?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Nov 17, 2019

Choose a reason for hiding this comment

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

That will be done as a follow up PR where we move the tutorials themselves into relevant folders. The idea is that teams/apps that own a product will move their tutorial and registration into their plugin. Tutorials that aren't app specific will remain as Kibana-type tutorials and those I'll move to the home plugin.

Comment on lines +55 to +56
const emptyContext = {};
const { error } = Joi.validate(specProvider(emptyContext), tutorialSchema);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Joi.validate(specProvider({}), tutorialSchema);?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Nov 17, 2019

Choose a reason for hiding this comment

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

That's the code as it was prior to the migration. If there's something specific you would like me to change that to, please let me know.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

Tested and works fine - space id shows up in the tutorial

Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks good, just had a couple nits. 👍

@@ -21,29 +21,44 @@ import _ from 'lodash';
import { getServices } from './kibana_services';
import { i18n } from '@kbn/i18n';

const baseUrlLP = getServices().addBasePath('/api/kibana/home/tutorials_LP');
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we ok with using uppercase in this filename? I believe all other files in the repo are lowercase.

server.registerTutorial(uwsgiMetricsSpecProvider);
server.registerTutorial(netflowSpecProvider);
server.registerTutorial(traefikLogsSpecProvider);
server.newPlatform.setup.plugins.home.tutorials.registerTutorial(systemLogsSpecProvider);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you feel like it would be beneficial for succinctness to loop through these?

[
  systemMetricsSpecProvider,
  /* ... */
  cockroachdbMetricsSpecProvider,
].forEach(provider =>
  server.newPlatform.setup.plugins.home.tutorials.registerTutorial(provider)
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tutorial registrations need to remain separate at this stage because they are going to move to their specific plugins (e.g. the apm tutorial registration will move to the apm plugin (or one of their plugins). When we've identified all the non-specific tutorials, we can consider looping over them. If you happen to know which ones the new home plugin should house (the non-app specific ones), please let us know!

@@ -0,0 +1,6 @@
{
"id": "home",
"version": "kibana",
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this over zoom, but I think we should make this version field consistent with other plugins to be semver, and also use kibanaVersion:

"version": "0.0.1",
"kibanaVersion": "kibana",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The work in this plugin is not yet complete and we can address this change in the next PR (#50838) that builds on this one.


public setup(core: CoreSetup) {
return {
tutorials: { ...this.tutorialsRegistry.setup(core) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I ask why we need to spread this instead of passing it directly?

Suggested change
tutorials: { ...this.tutorialsRegistry.setup(core) },
tutorials: this.tutorialsRegistry.setup(core),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to expose all the methods exposed in the setup of the service.


public start() {
return {
tutorials: { ...this.tutorialsRegistry.start() },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tutorials: { ...this.tutorialsRegistry.start() },
tutorials: this.tutorialsRegistry.start(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we spread the start contract for consistency with the setup method, should any further methods be added to the start contract of the service in the future.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@TinaHeiligers TinaHeiligers merged commit 04425ff into elastic:master Nov 18, 2019
nickofthyme pushed a commit to nickofthyme/kibana that referenced this pull request Nov 18, 2019
* [NP:Kibana:homeAPI] initializes a new plugin (WIP)

* Typing

* New plugin tutorials not needing server working

* Retains legacy tutorial registration and adds new route for new platform tutorial plugin registrations

* Adds comment on where to pre-register general non-plugin specific tutorials

* Converts TutorialsPlugin to TutorialsRegistry service in new Home plugin

* Changes call to location of registerTutorial in registerTutorials

* Adds console log for the home plugin that's returning an empty object

* Removes async from setup and start methods in the home plugin and the tutorials service

* Starts writing tests and creating mocks

* Adds basic tests for TutorialRegistry service

* Adds basic tests for TutorialRegistry service

* Adds test for route

* Adds mocks and tests for the home plugin

* Adds home plugin to security plugin and registers scoped tutorials service

* Removes incorrect addition to src core server

* Fixes type errors

* Deletes unused code

* Deletes duplicate golangMetricsSpecProvider registration

* Nests tutorials service in a tutorials key in the home plugin, changes api http routes for legacy platform and NP

* Changes url variable names
TinaHeiligers added a commit to TinaHeiligers/kibana that referenced this pull request Nov 18, 2019
* [NP:Kibana:homeAPI] initializes a new plugin (WIP)

* Typing

* New plugin tutorials not needing server working

* Retains legacy tutorial registration and adds new route for new platform tutorial plugin registrations

* Adds comment on where to pre-register general non-plugin specific tutorials

* Converts TutorialsPlugin to TutorialsRegistry service in new Home plugin

* Changes call to location of registerTutorial in registerTutorials

* Adds console log for the home plugin that's returning an empty object

* Removes async from setup and start methods in the home plugin and the tutorials service

* Starts writing tests and creating mocks

* Adds basic tests for TutorialRegistry service

* Adds basic tests for TutorialRegistry service

* Adds test for route

* Adds mocks and tests for the home plugin

* Adds home plugin to security plugin and registers scoped tutorials service

* Removes incorrect addition to src core server

* Fixes type errors

* Deletes unused code

* Deletes duplicate golangMetricsSpecProvider registration

* Nests tutorials service in a tutorials key in the home plugin, changes api http routes for legacy platform and NP

* Changes url variable names
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 18, 2019
…-fallback

* 'master' of github.com:elastic/kibana: (116 commits)
  [Maps] move apply global filter settting from layer to source (elastic#50523)
  [SIEM] Fix: Empty `Source` / `Destination` shown when only ports are populated (elastic#50843)
  [Maps] Delay vector tile layer syncing until spritesheet is loaded (elastic#48955)
  [Maps] prevent users from overflowing URL when filtering by shape (elastic#50747)
  [DOCS] Mark Beats central management as discontinued (elastic#49423)
  [page_objects/common_page] convert to ts (elastic#50771)
  [NP Kibana Migrations ] kibana plugin home (elastic#50444)
  [DOCS] Shareables naming convention (elastic#50497)
  [ML] DF Analytics - auto-populate model_memory_limit (elastic#50714)
  Increase alerting test stability and reduce flakiness (elastic#50246)
  [ML] Remaning new_job_new folder (elastic#50917)
  [Telemetry] Show opt-in changes for OSS users (elastic#50831)
  [ML] Fix lat_long anomalies table links menu and value formatting (elastic#50916)
  [Dev] Fix serialising a really big string (elastic#50915)
  Better explanation about the Prettier recommendation (extension vs. NPM module) (elastic#50629)
  [Monitoring] Use a basic monitoring user for tests (elastic#47865)
  [Monitoring] Gracefully handle issue with filebeat indices (elastic#48929)
  [Monitoring] Improve permissions required around setup mode (elastic#50421)
  Additional validation for elasticsearch username (elastic#48247)
  Revert changes to use_kibana_ui_setting (elastic#50877)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/server/request.test.ts
TinaHeiligers added a commit that referenced this pull request Nov 18, 2019
* [NP Kibana Migrations ] kibana plugin home (#50444)
@TinaHeiligers TinaHeiligers deleted the NP_kibana_plugin_home branch October 14, 2020 15:57
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.

6 participants