Migrate logstash frontend to KP by joshdover · Pull Request #63417 · elastic/kibana

joshdover · 2020-04-13T22:39:27Z

Summary

Closes #60937

This completed the logstash plugin migration to the Kibana Platform:

Removes all Angular code
Uses KP dependencies in place of legacy ones
Moves UI code to KP plugin
Deletes legacy logstash plugin

TODO:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

x-pack/plugins/logstash/public/services/license/logstash_license_service.js

...ck/legacy/plugins/logstash/public/lib/update_management_sections/update_logstash_sections.js

x-pack/plugins/logstash/public/sections/pipeline_edit_view.tsx

joshdover · 2020-04-14T22:53:23Z

x-pack/plugins/logstash/public/sections/index.tsx

+    // When monitoring is migrated this should be fetched from monitoring's plugin contract
+    core.injectedMetadata.getInjectedVar('monitoringUiEnabled'),


Not sure how to make sure this doesn't get missed when #62908 is merged. I will coordinate with @igoristic to make sure this doesn't break.

x-pack/plugins/logstash/public/plugin.ts

elasticmachine · 2020-04-15T13:56:37Z

Pinging @elastic/kibana-platform (Team:Platform)

mshustov

Create pipeline functionality is broken at the moment due to double basePath in the url. FTR does not define basePath, so CI is wrongly green

x-pack/plugins/logstash/public/plugin.ts

mshustov · 2020-04-15T16:15:11Z

x-pack/plugins/logstash/public/plugin.ts

+      order: 10,
+      mount: async params => {
+        const [coreStart] = await core.getStartServices();
+        const { renderApp } = await import('./sections');


Should we unify the pattern of the mounter declaration? Place in a folder structure, name, etc? Not a blocker for the current work.

We actually do have a folder convention for 1st class applications, though I wasn't sure if it should apply to management sections. It looks like other plugins are using this convention for sections as well, so I'll move everything to that for now.

They do. However, we might want to separate the application & management section folders. Also, we could introduce a convention to place mounter functions in the *_mounter file to unify the declarations.

x-pack/plugins/logstash/public/plugin.ts

x-pack/plugins/logstash/public/sections/pipeline_edit_view.tsx

mshustov · 2020-04-15T16:57:14Z

x-pack/plugins/logstash/public/services/monitoring/monitoring_service.js

@@ -32,16 +30,16 @@ export class MonitoringService {
      .then(cluster => {
        const url = `${this.basePath}/v1/clusters/${cluster.uuid}/logstash/pipeline_ids`;


this also should move to monitoring service maybe? it's not good that logstash calls monitoring plugin endpoint

Opened an issue: #63931. Will be easier to fix after monitoring plugin has migrated.

x-pack/plugins/logstash/public/models/pipeline_list_item/pipeline_list_item.js

x-pack/plugins/logstash/public/sections/index.tsx

ycombinator

Smoke tested this PR. Functionally LGTM. Thanks, @joshdover!

…ui-services

joshdover · 2020-04-19T19:41:23Z

Base path issue fixed!

base path problem fixed

pgayvallet

Some very minor nits. Feel free to ignore them if outside of the scope of the migration.

Seems like the jest test failure is caused because

kibana/src/plugins/management/public/management_service.test.ts

Lines 32 to 35 in d6a500c

    
           expect(service.getAllSections().length).toEqual(3); 
        
           expect(service.getSection('kibana')).not.toBeUndefined(); 
        
           expect(service.getSection('logstash')).not.toBeUndefined(); 
        
           expect(service.getSection('elasticsearch')).not.toBeUndefined();

needs to be adapted as the logstash section is no longer registered from legacy.

x-pack/plugins/logstash/public/application/pipeline_edit_view.tsx

pgayvallet · 2020-04-20T07:33:08Z

x-pack/plugins/logstash/public/application/pipeline_edit_view.tsx

+  const close = useCallback(() => {
+    history.push('/');
+  }, [history]);
+  const open = useCallback(
+    (newId: string) => {
+      history.push(`/pipeline/${newId}/edit`);
+    },
+    [history]
+  );


NIT: useCallback make sense for onRetry (even if debatable), but it seems unnecessary here.

Correct me if I'm wrong (I'm still new to hooks), but I thought the purpose of useCallback was to avoid creating a new function on each render, possibly triggering unnecessary re-renders of the child components. I'm basically just using this for memoization.

Yea you are right. Sorry, I thought history was also passed down to the child component (which would have made the useCallback usage useless) but it is not. All good :)

pgayvallet · 2020-04-20T07:46:17Z

x-pack/plugins/logstash/public/services/license/logstash_license_service.js

+  }
+
+  get isSecurityEnabled() {
+    return this.license.getFeature(`security`).isEnabled;


NIT: ` -> '

Can't use arrow functions on get properties (if that's what you meant).

x-pack/plugins/logstash/public/plugin.ts

mshustov · 2020-04-20T15:06:08Z

x-pack/plugins/logstash/public/services/license/logstash_license_service.js

+   * Rejects if the plugin is not available due to license.
+   */
+  checkValidity() {
+    return new Promise((resolve, reject) => {


It's one of the weirdest ways to check a boolean value I've ever seen. 😄

Yeah maybe I'm err'ing too much on the side of "don't changing anything in this plugin that we don't have to"... Just worried about lack of unit test coverage.

…ui-services

kibanamachine · 2020-04-20T22:33:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d9b5162

History

💔 Build #41763 failed 961e157
💚 Build #40841 succeeded e5ad3da
💚 Build #40834 succeeded ca6c71a
💔 Build #40749 failed cabc7745561686d6cb413b7f2f1423277ffc5f50
💔 Build #40543 failed c74861b61716731c1e53a5ddf7c48d7539753d3c

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

mattkime

App Arch changes lgtm

pgayvallet · 2020-04-21T05:30:04Z

x-pack/plugins/logstash/public/application/pipeline_edit_view.tsx

+  const close = useCallback(() => {
+    history.push('/');
+  }, [history]);
+  const open = useCallback(
+    (newId: string) => {
+      history.push(`/pipeline/${newId}/edit`);
+    },
+    [history]
+  );


Yea you are right. Sorry, I thought history was also passed down to the child component (which would have made the useCallback usage useless) but it is not. All good :)

mattkime mentioned this pull request Apr 14, 2020

[New Platform Migration]: Management - Shim apps #47431

Closed

19 tasks

joshdover added 2 commits April 14, 2020 15:59

WIP logstash ui migration

a6481c4

Move ui to KP

ca6c71a

joshdover force-pushed the logstash/migrate-ui-services branch from cabc774 to ca6c71a Compare April 14, 2020 22:34

joshdover commented Apr 14, 2020

View reviewed changes

small tweaks

e5ad3da

joshdover commented Apr 14, 2020

View reviewed changes

x-pack/plugins/logstash/public/plugin.ts Outdated Show resolved Hide resolved

joshdover added Feature:Logstash Pipelines Logstash Pipeline UI related Feature:NP Migration Team:Logstash Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.8.0 labels Apr 15, 2020

joshdover marked this pull request as ready for review April 15, 2020 13:56

joshdover added the release_note:skip Skip the PR/issue when compiling release notes label Apr 15, 2020

mshustov previously requested changes Apr 15, 2020

View reviewed changes

mshustov mentioned this pull request Apr 16, 2020

Run platform functional tests with/without base-path #63672

Open

ycombinator approved these changes Apr 16, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into logstash/migrate-…

c625c79

…ui-services

joshdover mentioned this pull request Apr 19, 2020

Refactor logstash -> monitoring dependency to use plugin contracts instead of HTTP #63931

Closed

joshdover added 2 commits April 19, 2020 13:40

Fix double basePath bug

7372af1

PR comments

961e157

joshdover requested a review from a team as a code owner April 19, 2020 19:41

joshdover requested a review from mshustov April 19, 2020 19:41

pgayvallet reviewed Apr 20, 2020

View reviewed changes

mshustov reviewed Apr 20, 2020

View reviewed changes

x-pack/plugins/logstash/public/plugin.ts Outdated Show resolved Hide resolved

mshustov reviewed Apr 20, 2020

View reviewed changes

x-pack/plugins/logstash/public/plugin.ts Show resolved Hide resolved

mshustov reviewed Apr 20, 2020

View reviewed changes

mshustov approved these changes Apr 20, 2020

View reviewed changes

joshdover added 2 commits April 20, 2020 14:39

Merge remote-tracking branch 'upstream/master' into logstash/migrate-…

a7accad

…ui-services

PR comments + test fixes

d9b5162

mattkime approved these changes Apr 21, 2020

View reviewed changes

pgayvallet approved these changes Apr 21, 2020

View reviewed changes

joshdover merged commit f43d555 into elastic:master Apr 21, 2020

joshdover deleted the logstash/migrate-ui-services branch April 21, 2020 13:40

joshdover mentioned this pull request Apr 21, 2020

[7.x] Migrate logstash frontend to KP (#63417) #64072

Merged

joshdover added a commit to joshdover/kibana that referenced this pull request Apr 21, 2020

Migrate logstash frontend to KP (elastic#63417)

14c446b

joshdover added a commit that referenced this pull request Apr 21, 2020

[7.x] Migrate logstash frontend to KP (#63417) (#64072)

8d8d44f

		// When monitoring is migrated this should be fetched from monitoring's plugin contract
		core.injectedMetadata.getInjectedVar('monitoringUiEnabled'),

		@@ -32,16 +30,16 @@ export class MonitoringService {
		.then(cluster => {
		const url = `${this.basePath}/v1/clusters/${cluster.uuid}/logstash/pipeline_ids`;

	expect(service.getAllSections().length).toEqual(3);
	expect(service.getSection('kibana')).not.toBeUndefined();
	expect(service.getSection('logstash')).not.toBeUndefined();
	expect(service.getSection('elasticsearch')).not.toBeUndefined();

Conversation

joshdover commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Apr 15, 2020

Uh oh!

mshustov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ycombinator left a comment

Choose a reason for hiding this comment

Uh oh!

joshdover commented Apr 19, 2020

Uh oh!

pgayvallet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mshustov Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Apr 20, 2020

💚 Build Succeeded

History

Uh oh!

mattkime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

joshdover commented Apr 13, 2020 •

edited

Loading

mshustov left a comment •

edited

Loading

mshustov Apr 20, 2020 •

edited

Loading