Add support for Kibana 7.10.0 #144

annie3431 · 2020-12-02T20:58:21Z

Issue #, if available: N/A

Description of changes:

Bump plugin version to 1.12.0.0
Bump Kibana version to 7.10.0
Bump node version to 10.22.1
Changed year of license for multiple files
Remove index.js and public/app.js and have
Removed dependencies in package.json to avoid duplicate imports

Browser side changes:

Add public/index.ts, public/index_management_app.tsx, and public/plugin.ts to define entry point
Add CoreStart prop for components
Changes to toast notification to be added by core.notifications
Breadcrumbs of pages are now set by core.setBreadcrumbs
For APIs that require query string, a query object will be used as parameter instead of forming a query string. This is due to the http client function not decoding the query correctly with a query string.
Update the browser side services to replace legacy angularIHttpServiceand IHttpResponse with new HttpSetup and HttpResponse

Server side changes:

Add server/index.ts, server/plugin.ts as server side entry point and configuration
Update route definitions to support new platform
Update route handlers to support new platform
Some rollup data model fix in interface file

Other:

Add external link to documentation on create rollup page

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Need to update http url

…elasticsearch/index-management-kibana-plugin into kibana-migration

* change position of index-management in kibana side bar * remove logo from side bar menu

annie3431 · 2020-12-03T06:49:25Z

Output for running yarn run test:jest

Test Suites: 32 passed, 32 total
Tests:       167 passed, 167 total
Snapshots:   28 passed, 28 total
Time:        148.345 s, estimated 327 s
Ran all test suites.
✨  Done in 149.68s.

package.json

ohltyler · 2020-12-03T19:55:00Z

server/services/ManagedIndexService.ts

+      return response.custom({
+        statusCode: 200,
+        body: {
+          ok: true,
+          response: results,
+        },
+      });


Rather than response.custom, you can use the given response.ok which will return a statusCode of 200 by default.

Was using response.ok but resulted in a lot of type error. So changed back to response.custom. Thanks for the reminder.

ohltyler · 2020-12-03T20:03:57Z

public/pages/ChangePolicy/components/ChangeManagedIndices/ChangeManagedIndices.tsx

@@ -27,6 +27,7 @@ interface ChangeManagedIndicesProps {
  onChangeManagedIndices: (selectedManagedIndices: { label: string; value?: ManagedIndexItem }[]) => void;
  onChangeStateFilters: (stateFilter: { label: string }[]) => void;
  managedIndicesError: string;
+  core: CoreStart;


Since I see you are wrapping all of the UI components in a core services consumer, you should be able to access the services via context (like how AD does it here), rather than passing a core object via props. This would simplify your components from having to add a core prop for all components and/or subcomponents that need to access the core services for toasts, breadcrumbs, etc. More info here

In my mind, accessing object via Context or props depends on the usage frequency of the object.
If an object are utilized in most of the components from the parent to the child, passing that through props maybe looks better. While, if the object is only used in few components, especially the deepest component, accessing the object via Context like AD plugin does (and AD features in Alerting Kibana plugin as well) definitely much simpler.

Modified code to get core from context instead of passing props.

package.json

dbbaughe · 2020-12-03T20:40:41Z

public/pages/ChangePolicy/components/NewPolicy/NewPolicy.test.tsx

-        onStateSelectChange={() => {}}
-        selectedPoliciesError=""
-      />
+      <CoreServicesContext.Provider value={coreServicesMock}>


Did you need to wrap all of these in the provider since you're already passing it below as props? Should only need the provider if you're consuming the context internally in NewPolicy. And maybe look into Tylers comment, this seems like a lot of nested passing.

Changed to let components access core by context. So the passing core as props arguments are removed.

dbbaughe · 2020-12-03T20:41:42Z

public/pages/ChangePolicy/components/NewPolicy/NewPolicy.tsx

@@ -34,6 +34,7 @@ interface NewPolicyProps {
  onChangePolicy: (selectedPolicies: PolicyOption[]) => void;
  onChangeStateRadio: (optionId: string) => void;
  onStateSelectChange: (e: React.ChangeEvent<HTMLSelectElement>) => void;
+  core: CoreStart;


Should try to limit what you're passing down. We don't need this giant core object, we just need the notifications service. You can use that in the props and pass it as core.notifications in whoever renders this.

This is what I did in Alerting Kibana plugin inspired by Yan's previous comment: https://github.com/opendistro-for-elasticsearch/alerting-kibana-plugin/blob/master/public/pages/Main/Main.js#L61. Drew please feel free to leave comment, and I will also keep an eye on how core will be passed here in Index Management plugin.

Refactored code to get core from context. Removed arguments to pass core as props.

public/pages/Policies/containers/Policies/Policies.tsx

public/services/RollupService.ts

public/utils/helpers.ts

server/routes/rollups.ts

dbbaughe · 2020-12-03T21:57:18Z

server/routes/indices.ts

-type Server = Legacy.Server;
-
-export default function(server: Server, services: NodeServices) {
+export default function (services: NodeServices, router: IRouter) {


Was the validations a requirement of the new framework? If we are going from no validations -> validations can you just confirm each API works as expected on Kibana.

Tested. Everything should work fine.

But I don't think validation is required for new platform.

dbbaughe · 2020-12-03T21:58:15Z

public/plugin.ts

+ */
+
+import { AppMountParameters, CoreSetup, CoreStart, Plugin, PluginInitializerContext } from "../../../src/core/public";
+import { IndexManagementPluginSetup } from ".";


I don't think I've ever seen from "." 🤔

I think this is because the interfaces IndexManagementPluginSetup and IndexManagementPluginStart are defined in ./index.ts

server/services/RollupService.ts

zengyan-amazon and others added 22 commits November 10, 2020 09:27

Migration- routing working

6658ea9

Need to update http url

change position of index-management in kibana side bar (#140)

f1a750e

Fix services query and response type

7175358

Update policies.ts

8e9b18f

Update IndexService.ts

127fb5d

Clean up code

0edfa80

Clean up code

542e37e

Update tests

1cef08a

Update on tests

0dd4a26

fix some unit tests about core services

52f92fb

Initial commit for upgrade with rollup

3047aad

Merge branch 'kibana-migration' of https://github.com/opendistro-for-…

7e11bd2

…elasticsearch/index-management-kibana-plugin into kibana-migration

Kibana migration (#142)

52dd0d4

* change position of index-management in kibana side bar * remove logo from side bar menu

Add core to rollup and modify server side code

8465cf6

Merge branch 'master' into kibana-migration

4792dd2

Code fix on breadcrumbs

f979a4c

Fixes dependencies, scripts, configs

4303c94

Updates package json and kibana json

c9453ff

Fix routing

c687dbd

Change id to opendistro_index_management_kibana

f83b8bd

Update server routes validation

82139b4

Update RollupService.ts

98f21fa

annie3431 added the version support label Dec 2, 2020

annie3431 requested review from dbbaughe, bowenlan-amzn, ftianli-amzn, ohltyler and qreshi December 2, 2020 20:59

Annie added 2 commits December 2, 2020 14:10

Update to tests

328ec49

Fix getMappings function

591817d

Annie added 2 commits December 2, 2020 17:02

Update query string imports

4d289cf

Update tests and snapshots

c2b17a1

Annie added 2 commits December 2, 2020 22:53

Add copyright license to files

fd07056

Update .kibana-plugin-helpers.json

f7904eb

annie3431 added a commit that referenced this pull request Dec 3, 2020

Add link for PR #144

66c8a08

ohltyler reviewed Dec 3, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

ohltyler reviewed Dec 3, 2020

View reviewed changes

dbbaughe reviewed Dec 3, 2020

View reviewed changes

Annie added 13 commits December 3, 2020 14:44

Remove kibana version from package.json

b70ebee

Update Policies.tsx

4e455c0

Update RollupService.ts

5db134a

Update rollups.ts

64e7b4d

Update RollupService.ts

1b80563

Update components to use useContext instead of passing props to get core

d9007d6

Update CoreServicesContext usage

d82ec65

Update tests

599d65f

Remove unused import

a3a939a

Update helpers.ts

842cb61

Update helpers.ts

d42d036

Update rollup API validation

de156b3

Update README.md

4433c15

dbbaughe previously approved these changes Dec 4, 2020

View reviewed changes

Add external link to aggragation doc

c23cd8a

annie3431 dismissed dbbaughe’s stale review via c23cd8a December 4, 2020 23:33

ftianli-amzn approved these changes Dec 4, 2020

View reviewed changes

dbbaughe approved these changes Dec 5, 2020

View reviewed changes

annie3431 merged commit cb7fb8a into master Dec 5, 2020

annie3431 deleted the kibana-migration branch December 5, 2020 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Kibana 7.10.0 #144

Add support for Kibana 7.10.0 #144

annie3431 commented Dec 2, 2020 •

edited

Loading

annie3431 commented Dec 3, 2020

ohltyler Dec 3, 2020

annie3431 Dec 3, 2020

ohltyler Dec 3, 2020

ftianli-amzn Dec 3, 2020

annie3431 Dec 4, 2020

dbbaughe Dec 3, 2020

annie3431 Dec 4, 2020

dbbaughe Dec 3, 2020

ftianli-amzn Dec 3, 2020

annie3431 Dec 4, 2020

dbbaughe Dec 3, 2020

annie3431 Dec 4, 2020

annie3431 Dec 4, 2020

dbbaughe Dec 3, 2020

annie3431 Dec 4, 2020

Add support for Kibana 7.10.0 #144

Add support for Kibana 7.10.0 #144

Conversation

annie3431 commented Dec 2, 2020 • edited Loading

annie3431 commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annie3431 commented Dec 2, 2020 •

edited

Loading