Spaces -> Client to NP #54298

legrego · 2020-01-08T19:49:03Z

Summary

Migrates the bulk of the spaces client-side application to the New Platform

Out of scope: The "enter space path" is blocked on #50658

legrego · 2020-01-10T13:20:00Z

legrego · 2020-01-10T14:47:26Z

@azasypkin I'm going to pause work on this until #53880 merges, so that you're not waiting on two of my PRs to get yours in 😄. I'm torn between using the NP management service here, or doing that move in a different PR. Do you have an opinion from a review perspective?

I expect I'll have to introduce a router of some sort when I migrate to the NP version. I'm hoping it won't introduce a ton of extra complexity, but I haven't dug too far into this yet.

azasypkin · 2020-01-10T16:49:29Z

I'm torn between using the NP management service here, or doing that move in a different PR. Do you have an opinion from a review perspective?

Both options would work for me. The only reason you'd want to keep these separated is that we're the only consumers of the brand new Management API and it's still a bit buggy. I'm testing it and reporting the issues I found at the moment, nothing super critical though, just minor things here and there.

I expect I'll have to introduce a router of some sort when I migrate to the NP version. I'm hoping it won't introduce a ton of extra complexity, but I haven't dug too far into this yet.

Yep, we'll need a router, I started to use react-router-dom (will push changes in a few), but wanted to discuss with you as well since I assume we want to use similar approach for both Spaces and Security.

…ces/np-client

legrego · 2020-01-30T16:23:56Z

Resuming work on this!

x-pack/plugins/spaces/public/management/management_service.tsx

...lugins/kibana/public/management/sections/settings/components/default_component_registry.d.ts

x-pack/plugins/spaces/public/advanced_settings/advanced_settings_service.tsx

elasticmachine · 2020-02-03T18:11:07Z

Pinging @elastic/kibana-security (Team:Security)

azasypkin

Looks great! I've left a couple of nits and questions and also noted a minor i18n issue. I haven't tested it thoroughly yet as I expect you're going to incorporate latest changes from advanced settings NP migration, so I'm planning to test with these (but tell me if you're not going to do that, it's fine by me too).

...lugins/kibana/public/management/sections/settings/components/default_component_registry.d.ts

azasypkin · 2020-02-04T16:35:33Z

src/legacy/ui/public/new_platform/new_platform.karma_mock.js

@@ -154,6 +154,9 @@ export const npSetup = {
        }),
      },
    },
+    spaces: {


note: can you please explain why we need this? I seems spaces is the only x-pack plugin mentioned here and I'm wondering if we should have something similar in x-pack.

It was a while ago..I had Karma test failures, so I followed the instructions in the migration guide to "fix" it. I have a lot less in the LP plugin now though, so maybe I'm already past the issue. I'll remove and see what happens. If it fails, I should be able to introduce a mock within the spaces plugin itself.

kibana/src/core/MIGRATION.md

Lines 1564 to 1570 in 5d6dbf0

#### What about karma tests?

While our plan is to only provide first-class mocks for Jest tests, there are many legacy karma tests that cannot be quickly or easily converted to Jest -- particularly those which are still relying on mocking Angular services via `ngMock`.

For these tests, we are maintaining a separate set of mocks. Files with a `.karma_mock.{js|ts|tsx}` extension will be loaded _globally_ before karma tests are run.

It is important to note that this behavior is different from `jest.mock('ui/new_platform')`, which only mocks tests on an individual basis. If you encounter any failures in karma tests as a result of new platform migration efforts, you may need to add a `.karma_mock.js` file for the affected services, or add to the existing karma mock we are maintaining in `ui/new_platform`.

Update: the LP spaces plugin runs its legacy.ts as part of a hack, so that it runs for all LP plugins. The goal of legacy.ts is to call the NP registerLegacyAPI, so that the plugin can finish its initialization. In the case of these Karma tests, we need the npSetup static import provided by new_platform.karma.mock to have the spaces plugin registered, so that legacy.ts can call registerLegacyAPI.

So an alternative to placing this here, is to place a guard within legacy.ts, to only call registerLegacyAPI if the spaces plugin is actually defined. What do you think about that approach?

So an alternative to placing this here, is to place a guard within legacy.ts, to only call registerLegacyAPI if the spaces plugin is actually defined. What do you think about that approach?

On the surface it sounds like a better workaround to me, do you have any concerns regarding it?

And thanks for the explanation!

On the surface it sounds like a better workaround to me, do you have any concerns regarding it?

None at all! Resolved in 3f24902

azasypkin · 2020-02-04T16:44:20Z

src/plugins/management/public/mocks/index.ts

@@ -17,10 +17,20 @@
 * under the License.
 */

+const createSetupContract = () => ({


optional nit: can we use (): jest.Mocked<ManagementSetup> => ({ to make sure these are up to date when interface changes?

This causes the following to fail:

const managementSetup = managementSetupMock.createSetupContract(); managementSetup.sections.getSection.mockReturnValue(foo);

Because managementSetup.sections is of type SectionsServiceSetup, rather than jest.Mocked<SectionsServiceSetup>.

Do you know of a way to tell TS what we really want to do here? I couldn't get it to work, but didn't spend more than 15 minutes on it.

Hmm, I see. IIRC Platform team used something like DeeplyMockedKeys instead of jest.Mocked for cases like this, maybe that would work for us too... But feel free to ignore if it takes too much time to fix Management's mocks and types.

That worked perfectly, thanks for the tip!

azasypkin · 2020-02-04T17:00:03Z

x-pack/plugins/spaces/public/plugin.tsx

+  private managementService?: ManagementService;
+
+  public setup(core: CoreSetup, plugins: PluginsSetup) {
+    const serverBasePath = core.injectedMetadata.getInjectedVar('serverBasePath') as string;


question: as far as I remember it's equivalent of core.http.basePath.get()?

Good catch, yes!

Ah, actually it's not the same. There doesn't appear to be any logic to distinguish between the server base path and the space-aware base path in the client-side basePath service.

@elastic/kibana-platform does the client-side basePath service need the equivalent serverBasePath property that the corresponding server-side basePath service defines?

There doesn't appear to be any logic to distinguish between the server base path and the space-aware base path in the client-side basePath service.

Oh, that's a bummer. I think we were asking this question before ping @restrry

x-pack/plugins/spaces/public/plugin.tsx

x-pack/plugins/spaces/public/management/management_service.tsx

x-pack/plugins/spaces/public/management/management_service.test.ts

x-pack/plugins/spaces/public/management/spaces_management_app.tsx

...lugins/spaces/public/copy_saved_objects_to_space/copy_saved_objects_to_space_service.test.ts

x-pack/plugins/spaces/public/management/edit_space/delete_spaces_button.test.tsx

flash1293

Kibana app changes LGTM. Thanks a lot for the mocks!

Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>

…ces/np-client

elasticmachine · 2020-02-06T19:21:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2a01095

History

💔 Build #24917 failed d45e63d
💔 Build #24688 failed d6ffb18
💔 Build #24670 failed 35b81ea
💚 Build #24071 succeeded 73cc266
💚 Build #23792 succeeded 03a122f

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

legrego · 2020-02-07T16:05:28Z

@azasypkin I've addressed all feedback, with the exception of the missing serverBasePath property on the public http service. I reverted to using the deprecated injectedMetadata service for the time being

azasypkin

LGTM, great job! Tested locally and haven't noticed anything wrong apart from one thing related to manual edits of Edit Space page URL.

x-pack/plugins/spaces/public/management/management_service.tsx

x-pack/plugins/spaces/public/management/spaces_management_app.tsx

x-pack/plugins/spaces/public/management/edit_space/manage_space_page.tsx

x-pack/plugins/spaces/public/management/edit_space/enabled_features/enabled_features.tsx

legrego · 2020-02-10T12:01:03Z

@elasticmachine merge upstream

kibanamachine · 2020-02-10T13:29:20Z

💚 Build Succeeded

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

History

💚 Build #24993 succeeded 2a01095
💔 Build #24917 failed d45e63d
💔 Build #24688 failed d6ffb18
💔 Build #24670 failed 35b81ea
💚 Build #24071 succeeded 73cc266

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

peteharverson

ML edit LGTM.

snide

I did a scan of the scss changes. They look to be all renames with no code changes. Did not look at any of the other code.

* moves * updates to support spaces client in NP * fixing MLs import * update karma mock * remove unnecessary setup license * fix merge from master * moving management app registration to NP * move space selector app to NP * remove unused xpackMain legacy dependency * hide spaces management if not authorized * additional testing * additional cleanup * additional testing * use NP advancedSettings plugin * Apply suggestions from code review Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com> * start addressing PR feedback * reverting logic to determine serverBasePath * removing spaces mock * mantain BWC with old management links * add types to management mock * address remaining PR feedback Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

legrego added 6 commits January 8, 2020 09:07

moves

d2883da

updates to support spaces client in NP

8c4516d

fixing MLs import

cce8d1f

update karma mock

890b685

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

e0ac89c

remove unnecessary setup license

6c8c55d

Merge branch 'master' into spaces/np-client

33f04bd

legrego added the Feature:NP Migration label Jan 10, 2020

legrego added 5 commits January 30, 2020 07:43

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

f650303

fix merge from master

25fcc60

moving management app registration to NP

0a4198f

move space selector app to NP

295773c

Merge branch 'spaces/np-client' of github.com:legrego/kibana into spa…

1df82cf

…ces/np-client

legrego added 6 commits January 30, 2020 11:38

remove unused xpackMain legacy dependency

5bdf2b9

hide spaces management if not authorized

925ceed

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

602b11e

additional testing

03a122f

additional cleanup

95d390e

additional testing

73cc266

legrego commented Feb 3, 2020

View reviewed changes

legrego added release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.7.0 v8.0.0 labels Feb 3, 2020

legrego marked this pull request as ready for review February 3, 2020 18:11

legrego added 2 commits February 4, 2020 15:17

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

7350cfa

use NP advancedSettings plugin

dca42cc

azasypkin reviewed Feb 5, 2020

View reviewed changes

flash1293 approved these changes Feb 5, 2020

View reviewed changes

legrego and others added 9 commits February 5, 2020 12:55

Apply suggestions from code review

35b81ea

Co-Authored-By: Aleh Zasypkin <aleh.zasypkin@gmail.com>

Merge branch 'spaces/np-client' of github.com:legrego/kibana into spa…

e038ff1

…ces/np-client

start addressing PR feedback

4399b59

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

d6ffb18

reverting logic to determine serverBasePath

d45e63d

removing spaces mock

3f24902

mantain BWC with old management links

9ebe560

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

6fac7e4

add types to management mock

2a01095

elastic deleted a comment from kibanamachine Feb 6, 2020

azasypkin self-requested a review February 7, 2020 16:07

azasypkin approved these changes Feb 7, 2020

View reviewed changes

address remaining PR feedback

25008da

Merge branch 'master' into spaces/np-client

cdf2c10

peteharverson approved these changes Feb 10, 2020

View reviewed changes

mattkime approved these changes Feb 10, 2020

View reviewed changes

snide approved these changes Feb 10, 2020

View reviewed changes

legrego merged commit 974a51d into elastic:master Feb 10, 2020

legrego deleted the spaces/np-client branch February 10, 2020 14:16

legrego mentioned this pull request Feb 10, 2020

[7.x] Spaces -> Client to NP (#54298) #57200

Merged

jportner mentioned this pull request Mar 16, 2020

Kibana should tell the user she can't create a space with already existing name instead of just error #55040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces -> Client to NP #54298

Spaces -> Client to NP #54298

legrego commented Jan 8, 2020 •

edited

Loading

legrego commented Jan 10, 2020

legrego commented Jan 10, 2020

azasypkin commented Jan 10, 2020

legrego commented Jan 30, 2020

elasticmachine commented Feb 3, 2020

azasypkin left a comment

azasypkin Feb 4, 2020

legrego Feb 6, 2020

legrego Feb 6, 2020 •

edited

Loading

azasypkin Feb 6, 2020

azasypkin Feb 6, 2020

legrego Feb 6, 2020

azasypkin Feb 4, 2020

legrego Feb 6, 2020

azasypkin Feb 6, 2020

legrego Feb 6, 2020

azasypkin Feb 4, 2020

legrego Feb 5, 2020

legrego Feb 5, 2020 •

edited

Loading

azasypkin Feb 6, 2020

flash1293 left a comment

elasticmachine commented Feb 6, 2020

legrego commented Feb 7, 2020

azasypkin left a comment

legrego commented Feb 10, 2020

kibanamachine commented Feb 10, 2020

peteharverson left a comment

snide left a comment

	#### What about karma tests?

	While our plan is to only provide first-class mocks for Jest tests, there are many legacy karma tests that cannot be quickly or easily converted to Jest -- particularly those which are still relying on mocking Angular services via `ngMock`.

	For these tests, we are maintaining a separate set of mocks. Files with a `.karma_mock.{js\|ts\|tsx}` extension will be loaded _globally_ before karma tests are run.

	It is important to note that this behavior is different from `jest.mock('ui/new_platform')`, which only mocks tests on an individual basis. If you encounter any failures in karma tests as a result of new platform migration efforts, you may need to add a `.karma_mock.js` file for the affected services, or add to the existing karma mock we are maintaining in `ui/new_platform`.

Spaces -> Client to NP #54298

Spaces -> Client to NP #54298

Conversation

legrego commented Jan 8, 2020 • edited Loading

Summary

legrego commented Jan 10, 2020

legrego commented Jan 10, 2020

azasypkin commented Jan 10, 2020

legrego commented Jan 30, 2020

elasticmachine commented Feb 3, 2020

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego Feb 6, 2020 • edited Loading

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

legrego Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 6, 2020

💚 Build Succeeded

History

legrego commented Feb 7, 2020

azasypkin left a comment

Choose a reason for hiding this comment

legrego commented Feb 10, 2020

kibanamachine commented Feb 10, 2020

💚 Build Succeeded

History

peteharverson left a comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

legrego commented Jan 8, 2020 •

edited

Loading

legrego Feb 6, 2020 •

edited

Loading

legrego Feb 5, 2020 •

edited

Loading