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

Index Management new platform migration #49359

Merged

Conversation

alisonelizabeth
Copy link
Contributor

@alisonelizabeth alisonelizabeth commented Oct 25, 2019

This PR is the first phase of moving the Index Management plugin to the new platform.

How to review

The UX/UI has not changed, so the main focus should be around the NP setup as well as ensuring no regressions were introduced.

There are a few available NP APIs that still need to be adopted. I will address this in a separate PR.

  • [server] http.route
  • [server] elasticsearch

@alisonelizabeth alisonelizabeth added Feature:Index Management Index and index templates UI v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Oct 25, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alisonelizabeth alisonelizabeth changed the title [WIP] Index Management new platform migration Index Management new platform migration Nov 11, 2019
@alisonelizabeth alisonelizabeth marked this pull request as ready for review November 11, 2019 20:29
@@ -0,0 +1,166 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was largely borrowed from https://github.com/elastic/kibana/blob/master/src/plugins/es_ui_shared/public/request/request.ts, but updated to work with the NP http service.


return result;
}

export async function loadTemplateToClone(name: Template['name']) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was not being used

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Just reviewed the shims and plugin classes, things look good for the first part of the migration 👍


// Register management section and Angular route
registerManagementSection(management.getSection('elasticsearch'));
registerRoutes(core as CoreStart);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this cast is unnecessary

const pluginsSetup = {};

const __LEGACY: LegacySetup = {
router: createRouter(server, PLUGIN.ID, `${API_BASE_PATH}/`),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing the bulk of the work for migrating the server-side code in this plugin will be adapting the router interface to work with the New Platform's router.

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Overall this contribution looks in really good shape!

I gave a smoke test locally and was able to perform all actions I attempted, did not do an in-depth regression test but given that notifications and http are working I think we are mostly safe!

Left some non-blocking comments.

import { BASE_PATH } from '../../../common/constants';

class BreadcrumbService {
private chrome: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have types for this, from here: src/core/public/chrome/chrome_service.tsx. I'm guessing the ChromeStart interface is the one we want.

}

public showSuccessToast(title: string, text?: string) {
this.addToasts(title, 'success', text);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like the toasts service API has methods like addSuccess we could use (I think it may add an icon too - so if it's not what we want ignore this entirely).

Same for above warning and danger instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I somehow missed that. I'll look into it - thanks!

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 added this as an item to fix for the next phase of NP work.

indices: string[];
}

const handler: RouterRouteHandler = async (request, callWithRequest, h) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@joshdover is there a plan for migration of ../../../../../../server/lib/create_router? Specifically looking at h 😄 , but also the other values passed in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see the plan is to move over to http.route NP service, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my understanding

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@alisonelizabeth
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alisonelizabeth alisonelizabeth merged commit 03e2a68 into elastic:master Nov 15, 2019
@alisonelizabeth alisonelizabeth deleted the np_migration/index_management branch November 15, 2019 18:54
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 18, 2019
…her [skip ci]

* upstream/master: (54 commits)
  allows plugins to define validation schema for "enabled" flag (elastic#50286)
  Add retry to find.existsByDisplayedByCssSelector (elastic#48734)
  [i18n] integrate latest translations (elastic#50864)
  ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)
  Fix @reach/router types (elastic#50863)
  [ML] Adding ML node warning to overview and analytics pages (elastic#50766)
  Bump storybook dependencies (elastic#50752)
  [APM Replace usage of idx with optional chaining (elastic#50849)
  [SIEM] Fix eslint errors (elastic#49713)
  Improve "Browser client is out of date" error message (elastic#50296)
  [SIEM][Detection Engine] REST API improvements and changes from UI/UX feedback (elastic#50797)
  Move @kbn/es-query into data plugin - es-query folder (elastic#50182)
  Index Management new platform migration (elastic#49359)
  Increase retry for cloud snapshot to finish (elastic#50781)
  Removing EuiCode from inside EuiPanel (elastic#50683)
  [SIEM] Tests for search_after and bulk index (elastic#50129)
  Make babel understand TypeScript 3.7 syntax (elastic#50772)
  Fixing mocha tests and broken password change status codes (elastic#50704)
  [Canvas] Use compressed forms in sidebar (elastic#49419)
  Add labels to shell scripts in Jenkins (elastic#49657)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants