Spaces - Client NP Migration, Phase 1 #40856

legrego · 2019-07-11T12:34:01Z

Summary

This introduces a shim in the client-side spaces plugin to start migrating to the new platform.

This PR uses the following NP core/plugin services:

Chrome Nav Control
Feature Catalogue Registry

This PR also performs some ancillary cleanup/prep work for when the rest of the plugin is able to migrate:

Introduce NP "Plugin" in legacy world
Migrate Chrome Nav Control to NP
Remove SpacesNavState, an unnecessary angular service
Update SpacesManager to use NP dependencies (http and notifications)
Initialize SpacesManager once within NP, consume single instance everywhere
Remove almost all injected variables, replace with private API calls
Replace usages of ui/capabilities with the NP equivalent
Migrate Feature Catalogue Registration to NP

Resolves #46255

Out of scope

Management screens - ⛔️ blocked: relies on legacy Kibana plugin/uiExports
Space selector screen door - ⛔️ blocked: relies on legacy Kibana for rendering hidden apps

legrego · 2019-07-11T17:11:22Z

x-pack/legacy/plugins/telemetry/public/views/management/management.js

      const telemetryOptInProvider = Private(TelemetryOptInProvider);

      const spaceProps = {
        spacesEnabled,
      };

-      if (spacesEnabled) {


stray code, was not being used in TememetryForm component

elasticmachine · 2019-07-11T19:50:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-11T21:19:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-11T21:34:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

x-pack/legacy/plugins/spaces/server/routes/api/v1/spaces.ts

legrego · 2019-07-12T14:50:16Z

x-pack/legacy/plugins/spaces/public/views/space_selector/space_selector.tsx

@@ -25,7 +26,6 @@ import { Space } from '../../../common/model/space';
 import { SpaceCards } from '../components/space_cards';

 interface Props {
-  spaces?: Space[];


If this.props.spaces was provided, it was via injected variables, which have been removed as part of this PR. Now, the selector screen must request the list of spaces on mount.

legrego · 2019-07-12T14:52:24Z

x-pack/legacy/plugins/spaces/public/views/nav_control/components/spaces_menu.tsx

@@ -175,6 +187,14 @@ class SpacesMenuUI extends Component<Props, State> {
      </EuiContextMenuItem>
    );
  };
+
+  private rendePlaceholderMenuItem = (key: string | number): JSX.Element => {


Not strictly required as part of this migration, but it makes for a better user experience when loading the list of available spaces. We often take the near-immediate responses for granted when running locally, and forget that these things can take a noticeable amount of time when hosted in the real world.

++, it actually looks very nice (testing with throttled network connection) 👍

Suggested change

private rendePlaceholderMenuItem = (key: string | number): JSX.Element => {

private rendePlaceholderMenuItem = (key: string | number) => {

elasticmachine · 2019-07-12T14:55:50Z

Pinging @elastic/kibana-security

elasticmachine · 2019-07-12T14:59:52Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

legrego · 2019-07-12T15:10:17Z

retest

afgomez

👋! I pulled the branch locally to test the infra changes and I found a thing.

I have two spaces: default and afgomez. I'm normally in the second space.

The changes in the useKibanaSpaceId hook cause the log analysis page to misbehave (in the "Logs" app, the second tab).

This is what I got on master

This is what I got on this branch

As you can see the content of the page changes, showing the wrong results when using this branch. I added a console.log after this line to see the value of spaceId before each render.

I think the issue is that useKibanaSpaceId is now asynchronous. We either need to make sure that it's synchronous again or allow it to return undefined if the space is not loaded yet. Then the components can decide how to render their respective trees in the absense of a spaceId.

I hope the description of the issue is clear. Please reach out to me if you have any questions <3

legrego · 2019-12-10T19:49:38Z

@afgomez thanks so much for testing and leaving feedback!

I updated the hook locally to return string | undefined, but the fallout from that cascades throughout the app, to the point where I'm not comfortable making the changes on my own (or as part of this PR)

I'll give this some more thought, but I'm learning towards re-introducing the activeSpace injected var to the legacy platform so that you can continue to consume this as-is, but then your migration to the NP will potentially require you to handle the space id arriving in an async manner.

afgomez · 2019-12-11T09:14:18Z

your migration to the NP will potentially require you to handle the space id arriving in an async manner.

@legrego that should be OK. We will take it into account :)

Thanks!

…-control

legrego · 2019-12-11T14:25:28Z

@afgomez I restored the original functionality, and the log analysis UI appears to function correctly for me now. Would you mind double-checking for me when you get a chance?

afgomez

@legrego thanks for this! I tried again and it works like a charm.

Infra changes LGTM

legrego · 2019-12-13T17:07:35Z

@elasticmachine merge upstream

…o np/spaces-nav-control

…-control

azasypkin

Looks great, thanks for doing this! I've played with a number of basic Spaces related operations - everything worked as expected.

legrego · 2019-12-16T11:29:50Z

@elasticmachine merge upstream

elasticmachine · 2019-12-16T12:46:23Z

💚 Build Succeeded

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

History

💔 Build #15512 failed 884e545
💚 Build #15505 succeeded 7cfdcb1
💚 Build #15469 succeeded 5cbb6d9
💚 Build #14790 succeeded 71865a9
💚 Build #14754 succeeded 03f1687

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

* Spaces - Client NP Migration, Phase 1 (#40856) * shimming NP for spaces client-side plugin * refresh active space in nav control when updated * fix advanced settings screen * allow npStart from unauthed routes * use NP for deriving space management url * remove security's usage of SpacesManager * remove usages of ui/capabilities * fix tests * implement NP plugin interface * remove hack in favor of convention in migration guide * shim feature catalogue registration * streamline nav control, and handle async loading more gracefully * adding opaqueId * fixes from merge * fix merge from master * fixing merge from master * move _active_space route to NP * moving to the NP feature catalogue registry * moving setup to setup phase * optimizing active space retrieval * reverting test isolation change * Apply suggestions from code review Co-Authored-By: Aleh Zasypkin <[email protected]> * removing unnecessary PluginInitializerContext * updating advanced settings subtitle * using NP anonymousPaths service * additional nav_control_popover cleanup * additional cleanup * testing out onActiveSpaceChange$ property * make the linter happy * make the type checker happy * fixing types * fix merge from master * spaces LP init should run on all pages, not just the kibana app * address nits * fix infra/logs, and the spaces disabled scenario * fix typescript errors * revert changes to infra plugin * reintroducing activeSpace injected var for legacy plugins * fixing react deprecation warning and unhandled promise rejection * restore activeSpace default var * spaces does not need to check its own enabled status * fix from merge Co-authored-by: Elastic Machine <[email protected]> * fix backport merge

This reverts commit fcdaed9.

legrego added 2 commits July 11, 2019 08:30

shimming NP for spaces client-side plugin

f80f888

refresh active space in nav control when updated

452d592

This comment has been minimized.

Sign in to view

fix advanced settings screen

b41073c

This comment has been minimized.

Sign in to view

allow npStart from unauthed routes

1677dc4

legrego commented Jul 11, 2019

View reviewed changes

legrego added 3 commits July 11, 2019 13:17

use NP for deriving space management url

a014d72

remove security's usage of SpacesManager

b0bf4dd

remove usages of ui/capabilities

c46f283

This comment has been minimized.

Sign in to view

legrego added 2 commits July 11, 2019 14:49

fix tests

aed3888

implement NP plugin interface

2aa3b71

legrego added 2 commits July 11, 2019 16:17

remove hack in favor of convention in migration guide

b039a99

shim feature catalogue registration

8c161f2

streamline nav control, and handle async loading more gracefully

3a010f1

legrego commented Jul 12, 2019

View reviewed changes

x-pack/legacy/plugins/spaces/server/routes/api/v1/spaces.ts Outdated Show resolved Hide resolved

legrego commented Jul 12, 2019

View reviewed changes

legrego added Feature:New Platform Feature:Security/Spaces Platform Security - Spaces feature release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels Jul 12, 2019

afgomez suggested changes Dec 10, 2019

View reviewed changes

legrego added 2 commits December 10, 2019 16:14

revert changes to infra plugin

d771522

reintroducing activeSpace injected var for legacy plugins

0186d5f

legrego added 4 commits December 11, 2019 07:41

fixing react deprecation warning and unhandled promise rejection

5b2c16a

Merge branch 'master' of github.com:elastic/kibana into np/spaces-nav…

03f1687

…-control

restore activeSpace default var

e29437b

Merge branch 'master' of github.com:elastic/kibana into np/spaces-nav…

71865a9

…-control

afgomez approved these changes Dec 12, 2019

View reviewed changes

legrego requested a review from azasypkin December 12, 2019 19:56

elasticmachine and others added 6 commits December 13, 2019 09:11

Merge branch 'master' into np/spaces-nav-control

5cbb6d9

spaces does not need to check its own enabled status

2ff6cb8

Merge branch 'np/spaces-nav-control' of github.com:legrego/kibana int…

7cfdcb1

…o np/spaces-nav-control

Merge branch 'master' of github.com:elastic/kibana into np/spaces-nav…

bbfc0bd

…-control

fix from merge

ffb8a7b

Merge branch 'master' of github.com:elastic/kibana into np/spaces-nav…

884e545

…-control

azasypkin approved these changes Dec 13, 2019

View reviewed changes

Merge branch 'master' into np/spaces-nav-control

cb44020

legrego added the v7.6.0 label Dec 16, 2019

legrego merged commit fcdaed9 into elastic:master Dec 16, 2019

legrego deleted the np/spaces-nav-control branch December 16, 2019 13:31

legrego mentioned this pull request Dec 16, 2019

[7.x] Spaces - Client NP Migration, Phase 1 (#40856) #53113

Merged

brianseeders added a commit to brianseeders/kibana that referenced this pull request Dec 17, 2019

Revert "Spaces - Client NP Migration, Phase 1 (elastic#40856)"

40e40f0

This reverts commit fcdaed9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces - Client NP Migration, Phase 1 #40856

Spaces - Client NP Migration, Phase 1 #40856

legrego commented Jul 11, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

legrego Jul 11, 2019

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 11, 2019

legrego Jul 12, 2019

legrego Jul 12, 2019

azasypkin Oct 30, 2019

elasticmachine commented Jul 12, 2019

elasticmachine commented Jul 12, 2019

legrego commented Jul 12, 2019

afgomez left a comment

legrego commented Dec 10, 2019

afgomez commented Dec 11, 2019

legrego commented Dec 11, 2019

afgomez left a comment

legrego commented Dec 13, 2019

azasypkin left a comment

legrego commented Dec 16, 2019

elasticmachine commented Dec 16, 2019

	private rendePlaceholderMenuItem = (key: string \| number): JSX.Element => {
	private rendePlaceholderMenuItem = (key: string \| number) => {

Spaces - Client NP Migration, Phase 1 #40856

Spaces - Client NP Migration, Phase 1 #40856

Conversation

legrego commented Jul 11, 2019 • edited Loading

Summary

Out of scope

This comment has been minimized.

This comment has been minimized.

legrego Jul 11, 2019

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

legrego Jul 12, 2019

Choose a reason for hiding this comment

legrego Jul 12, 2019

Choose a reason for hiding this comment

azasypkin Oct 30, 2019

Choose a reason for hiding this comment

elasticmachine commented Jul 12, 2019

elasticmachine commented Jul 12, 2019

💔 Build Failed

legrego commented Jul 12, 2019

afgomez left a comment

Choose a reason for hiding this comment

legrego commented Dec 10, 2019

afgomez commented Dec 11, 2019

legrego commented Dec 11, 2019

afgomez left a comment

Choose a reason for hiding this comment

legrego commented Dec 13, 2019

azasypkin left a comment

Choose a reason for hiding this comment

legrego commented Dec 16, 2019

elasticmachine commented Dec 16, 2019

💚 Build Succeeded

History

legrego commented Jul 11, 2019 •

edited

Loading