-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Index Management new platform migration #49359
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
💔 Build Failed
|
b2f518f
to
739edd8
Compare
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed
|
9d47441
to
534bd0c
Compare
💔 Build Failed |
@@ -0,0 +1,166 @@ | |||
/* |
There was a problem hiding this comment.
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']) { |
There was a problem hiding this comment.
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
@elasticmachine merge upstream |
💔 Build Failed |
@elasticmachine merge upstream |
💔 Build Failed |
There was a problem hiding this 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); |
There was a problem hiding this comment.
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}/`), |
There was a problem hiding this comment.
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.
There was a problem hiding this 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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my understanding
@elasticmachine merge upstream |
💔 Build Failed |
💔 Build Failed |
@elasticmachine merge upstream |
💚 Build Succeeded |
…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) ...
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.