-
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] Move to new platform "plugins" folder #58109
[Index management] Move to new platform "plugins" folder #58109
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
import { plugin as initServerPlugin, Dependencies } from './server'; | ||
|
||
export type ServerFacade = Legacy.Server; | ||
|
||
export function indexManagement(kibana: 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.
This file needs to stay until all dependent apps have migrated to the "plugins" folder.
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.
nice job @sebelga! another app to check off the migration list ✔️ 🎉
I left a couple questions in the code. I found a few issues with CCR and rollups when I disabled the index_management
plugin that I think need to be addressed before merging. Also, it looks like there are some minor changes you made to the mappings editor (I did not comment on all of them) that seem unrelated to the migration. If that's the case, maybe it should be addressed in a separate PR?
Otherwise, code LGTM. I tested CCR, ILM and rollups integrations and everything worked as expected. I also went through all the actions for index and index templates.
Since I'll be out the next few days, maybe @jloleysens can take another look at this when you're ready?
@@ -77,10 +77,10 @@ export class RollupsServerPlugin implements Plugin<void, void, any, any> { | |||
} | |||
|
|||
if ( | |||
serverShim.plugins.index_management && | |||
serverShim.plugins.index_management.addIndexManagementDataEnricher | |||
serverShim.plugins.indexManagement && |
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: can indexManagement
be moved out of the serverShim
now?
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.
Good call! 👍
@@ -24,7 +24,7 @@ export const DocumentFields = React.memo(() => { | |||
if (editorType === 'json') { |
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.
are changes to this file related to the migration?
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.
Yes, all the changes about adding/updating the array of dependencies of useCallback
and useEffect
were needed to fix a new rule in eslint
(which is great as it helps us fix unnecessary re-renders).
@@ -56,15 +56,21 @@ export const AnalyzerParameterSelects = ({ | |||
}); |
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.
same here
@@ -110,7 +110,7 @@ export const ConfigurationForm = React.memo(({ defaultValue }: Props) => { | |||
}); |
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.
are changes to this file related to the migration?
@@ -61,6 +67,9 @@ export class IndexMgmtServerPlugin implements Plugin<IndexMgmtSetup, void, any, | |||
indexDataEnricher: { | |||
add: this.indexDataEnricher.add.bind(this.indexDataEnricher), | |||
}, | |||
__legacy: { |
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.
is this needed?
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.
A left over! I will remove it 👍
server.expose('addIndexManagementDataEnricher', serverPublicApi.indexDataEnricher.add); | ||
isEnabled(config: any) { | ||
return ( | ||
config.get('xpack.index_management.enabled') && config.get('xpack.index_management.enabled') |
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.
should this just be return config.get('xpack.index_management.enabled')
?
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.
Lol 😄
config(Joi: any) { | ||
return Joi.object({ | ||
// display menu item | ||
ui: Joi.object({ |
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.
Is this needed? I don't think xpack.index_management.ui.enabled
is an existing setting.
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.
Actually, I'm not quite sure I follow why the legacy config is needed, since it didn't exist previously. Can you explain?
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 thought those were necessary but apparently not. I think that previously we forgot to add them though (at least the enabled
config) 😊
I removed everything but configPrefix
and id
which seems to be the minimum for the dependent apps to not complain.
@@ -20,7 +20,7 @@ import { addAllExtensions } from './np_ready/extend_index_management'; | |||
if (chrome.getInjected('ilmUiEnabled')) { | |||
// We have to initialize this outside of the NP lifecycle, otherwise these extensions won't | |||
// be available in Index Management unless the user visits ILM first. | |||
addAllExtensions(); | |||
addAllExtensions((npSetup.plugins as any).indexManagement.extensionsService); |
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 think this will cause issues if the index_management plugin is disabled.
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.
Good catch, I added a check. I think this bug is present on master then.. but not for long 😊
@@ -19,4 +19,4 @@ export const followerBadgeExtension = { | |||
filterExpression: 'isFollowerIndex:true', | |||
}; | |||
|
|||
extensionsService.addBadge(followerBadgeExtension); | |||
npSetup.plugins.indexManagement.extensionsService.addBadge(followerBadgeExtension); |
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 think this is going to cause issues if index management is disabled.
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.
Good catch, I added a check.
…x-management-plugins-folder
Thanks for the review @alisonelizabeth !
Great catch, I fixed them. I think those are already present on master as I did not touch the logic and simply updated the pointer to the extensions.
All the changes were required to pass |
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.
Code changes look good, could not spot a regression during local testing.
@elasticmachine merge upstream |
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.
Not a requirement, but you might want to consider changing the way your Sass imports into these files now that the optimizer is improved. https://github.com/elastic/kibana/tree/master/packages/kbn-optimizer
- Each sass file can now be imported directly into the JS component, rather than bundled into a giant index at the app root.
- If you do this, it would make sense to remove the
_
prefix on the sass files you choose to import directly. The underscore denotes a partial that is imported into another file that gets compiled.
The benefits to do this (not just in this plugin, but all through Kibana) is that you only load the CSS you need.
Thanks for the review @snide ! I will keep it as a note but I prefer to keep this PR focused on moving to the "plugins" folder and have a separate PR for this enhancement. It looks like a powerful feature indeed! |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…re/files-and-filetree * 'master' of github.com:elastic/kibana: (174 commits) [SIEM] Fix unnecessary re-renders on the Overview page (elastic#56587) Don't mutate error message (elastic#58452) Fix service map popover transaction duration (elastic#58422) [ML] Adding filebeat config to file dataviz (elastic#58152) [Uptime] Improve refresh handling when generating test data (elastic#58285) [Logs / Metrics UI] Remove path prefix from ViewSourceConfigur… (elastic#58238) [ML] Functional tests - adjust classification model memory (elastic#58445) [ML] Use event.timezone instead of beat.timezone in file upload (elastic#58447) [Logs UI] Unskip and stabilitize log column configuration tests (elastic#58392) [Telemetry] Separate the license retrieval from the stats in the usage collectors (elastic#57332) hide welcome screen for cloud (elastic#58371) Move src/legacy/ui/public/notify/app_redirect to kibana_legacy (elastic#58127) [ML] Functional tests - stabilize typing during df analytics creation (elastic#58227) fix short url in spaces (elastic#58313) [SIEM] Upgrades cypress to version 4.0.2 (elastic#58400) [Index management] Move to new platform "plugins" folder (elastic#58109) [kbn/optimizer] disable parallelization in terser plugin (elastic#58396) [Uptime] Delete useless try...catch blocks (elastic#58263) [Uptime] Use scripted metric for snapshot calculation (elastic#58247) (elastic#58389) [APM] Stabilize agent configuration API (elastic#57767) ... # Conflicts: # src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
# Conflicts: # x-pack/plugins/index_management/public/application/components/mappings_editor/mappings_state.tsx
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
This PR moves the index_management app to the "plugins" new platform folder.
The changes are mainly files that have been moved and re-connect the 3 dependent apps:
Making sure both server (see the 3 extensions
![Screen Shot 2020-02-20 at 16 28 56](https://user-images.githubusercontent.com/2854616/74929984-ab5eb580-5402-11ea-8ba2-9094164e4e7d.png)
ilm
,isRollupIndex
,isFollowerIndex
added to the index data):And client extensions still work
@cuff-links It be awesome if you could run the test plan on the app! 👍