Spaces - Migrate to NP Saved Objects Service #58716

legrego · 2020-02-27T12:03:15Z

Summary

Removes all usages of the legacy saved objects service from the Spaces plugin:

Client wrapper registration
Saved Object type registration
"import"/"export" functionality for Copy to space feature
Repository creation for the SpacesClient and create_default_space handler
error handling

legrego · 2020-03-04T17:06:31Z

@elasticmachine merge upstream

…/np-sos

legrego · 2020-03-05T01:09:19Z

@elasticmachine merge upstream

legrego · 2020-03-05T13:26:44Z

@elasticmachine merge upstream

legrego · 2020-03-05T18:22:57Z

@elasticmachine merge upstream

legrego · 2020-03-05T20:13:15Z

@elasticmachine merge upstream

legrego · 2020-02-28T14:34:55Z

x-pack/plugins/spaces/server/saved_objects/migrations/migrate_6x.test.ts

@@ -11,10 +11,12 @@ describe('migrateTo660', () => {
    expect(
      migrateToKibana660({
        id: 'space:foo',
+        type: 'space',


note type property added solely to comply with the type definition

legrego · 2020-03-05T21:32:44Z

src/core/server/saved_objects/saved_objects_service.ts

+  /**
+   * Returns the maximum number of objects allowed for import or export operations.
+   */
+  getImportExportObjectLimit: () => number;


note exposing as part of the start contract for easier consumption by the Spaces plugin. I already need core start services, so it made more sense for me to grab this from here too.

I exposed it in the setup contract because that's a value that could be needed from the plugin's setup phase.
We usually don't want to expose APIs in both setup and start methods (even if we do have exceptions to that rule)

Now that's it's done however, I think it is alright, unless someone says otherwise. @rudolf ?

We usually don't want to expose APIs in both setup and start methods (even if we do have exceptions to that rule)

Ah, I knew we had several examples in core where we did this, but didn't realize it was an exceptional case, and not a pattern to be repeated. Thanks for the heads-up!

In general, we want there to be one way and one way only to do things with Core API's to minimise confusion. This makes sense for API methods that are already bound to a specific lifecycle, but I think we can possibly be less strict with read-only config values.

However in this case I don't think it should impact the code quality a lot, so I'll leave another comment for a suggested way to refactor it.

legrego · 2020-03-05T21:34:53Z

x-pack/plugins/spaces/server/lib/copy_to_spaces/lib/saved_objects_client_opts.ts

-  }
-  return doc;
-}
+import { SavedObjectsClientProviderOptions } from 'src/core/server';


Bad rename detection by git. This was previously defined in x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.ts, but I extracted it to its own file after for easier sharing between x-pack/plugins/spaces/server/lib/copy_to_spaces/copy_to_spaces.ts and x-pack/plugins/spaces/server/lib/copy_to_spaces/resolve_copy_conflicts.ts

legrego · 2020-03-05T21:36:22Z

x-pack/plugins/spaces/server/routes/api/__fixtures__/create_copy_to_space_mocks.ts

@@ -0,0 +1,43 @@
+/*


note these functions were split out from x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts and adjusted accordingly

legrego · 2020-03-05T21:36:50Z

x-pack/plugins/spaces/server/routes/api/__fixtures__/create_mock_so_service.ts

@@ -0,0 +1,84 @@
+/*


note this was split out from x-pack/plugins/spaces/server/routes/api/__fixtures__/create_legacy_api.ts and adjusted accordingly

legrego · 2020-03-05T21:37:55Z

x-pack/plugins/spaces/server/routes/api/external/copy_to_space.test.ts

    const savedObjectsRepositoryMock = createMockSavedObjectsRepository(spacesSavedObjects);

+    (exportSavedObjectsToStream as jest.Mock).mockImplementation(
+      createExportSavedObjectsToStreamMock()


note these create*Mock functions are only used here, but I kept their definitions under __fixtures__ in an attempt to make this test suite halfway readable. I can inline them if you think it makes more sense to do so.

I think the way you have it now is good.

legrego · 2020-03-05T21:39:38Z

x-pack/plugins/spaces/server/saved_objects/mappings.ts

+
+import { deepFreeze } from '../../../../../src/core/utils';
+
+export const SpacesSavedObjectMappings = deepFreeze({


We defined this as a json file in the LP, but since NP has actual type definitions, I thought it made sense to convert this to TypeScript. I don't have a super strong opinion about this however.

elasticmachine · 2020-03-06T16:40:03Z

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

legrego · 2020-03-24T01:40:27Z

@elasticmachine merge upstream

…/np-sos

legrego · 2020-04-01T19:48:45Z

@elasticmachine merge upstream

legrego · 2020-04-01T21:49:44Z

@elasticmachine merge upstream

legrego · 2020-04-02T12:13:15Z

@elasticmachine merge upstream

…/np-sos

legrego · 2020-04-02T23:37:26Z

@elasticmachine merge upstream

legrego · 2020-04-03T01:41:04Z

@elasticmachine merge upstream

…/np-sos

kibanamachine · 2020-04-03T13:34:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9cff337

History

💔 Build #38307 failed 424721e
💚 Build #38297 succeeded 7d643a1
💔 Build #38273 failed 9de6355
💔 Build #38176 failed 7af1759
💔 Build #38126 failed 264fd39

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

* use NP saved objects service for type and wrapper registration * simplifying * additional testing * revert snapshot changes * removing dependency on legacy saved objects service * consolidate mocks * fixing imports * addrress PR feedback * remove unused docs * adjust tests for updated corestart contract * address test flakiness * address flakiness, part 2 * address test flakiness Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* use NP saved objects service for type and wrapper registration * simplifying * additional testing * revert snapshot changes * removing dependency on legacy saved objects service * consolidate mocks * fixing imports * addrress PR feedback * remove unused docs * adjust tests for updated corestart contract * address test flakiness * address flakiness, part 2 * address test flakiness Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

legrego force-pushed the spaces/np-sos branch from d69332a to c011ef9 Compare February 28, 2020 14:32

legrego added 5 commits March 4, 2020 07:19

use NP saved objects service for type and wrapper registration

ac9c99b

simplifying

e9b1ecc

additional testing

0c5a5ef

revert snapshot changes

0bb63a1

removing dependency on legacy saved objects service

d3f86ab

legrego force-pushed the spaces/np-sos branch from acc91b3 to d3f86ab Compare March 4, 2020 14:55

legrego changed the title ~~Spaces - use NP saved objects service for type and wrapper registration~~ Spaces - Migrate to NP Saved Objects Service Mar 4, 2020

elasticmachine and others added 4 commits March 4, 2020 12:06

Merge branch 'master' into spaces/np-sos

a54df5b

consolidate mocks

f6fb69a

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

a70c444

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

c92482b

…/np-sos

Merge branch 'master' into spaces/np-sos

33f8d28

Merge branch 'master' into spaces/np-sos

3963fc8

elasticmachine and others added 3 commits March 5, 2020 15:13

Merge branch 'master' into spaces/np-sos

5cc960e

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

86dd76c

fixing imports

f28b869

legrego commented Mar 6, 2020

View reviewed changes

legrego added Feature:NP Migration 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! v7.7.0 v8.0.0 labels Mar 6, 2020

elasticmachine and others added 4 commits March 23, 2020 21:40

Merge branch 'master' into spaces/np-sos

ca37e58

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

03b5dcd

adjust tests for updated corestart contract

40aa2c7

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

bbb958b

…/np-sos

Merge branch 'master' into spaces/np-sos

514e90e

Merge branch 'master' into spaces/np-sos

d3fd158

Merge branch 'master' into spaces/np-sos

264fd39

legrego added v7.8.0 and removed v7.7.0 labels Apr 2, 2020

legrego added 2 commits April 2, 2020 11:17

address test flakiness

14fb7b2

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

7af1759

…/np-sos

legrego requested a review from a team as a code owner April 2, 2020 15:18

address flakiness, part 2

9de6355

Merge branch 'master' into spaces/np-sos

7d643a1

elasticmachine and others added 4 commits April 2, 2020 21:41

Merge branch 'master' into spaces/np-sos

424721e

address test flakiness

aacb8ad

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

5a73d58

…/np-sos

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

9cff337

legrego merged commit 37c8262 into elastic:master Apr 3, 2020

legrego deleted the spaces/np-sos branch April 3, 2020 13:50

legrego mentioned this pull request Apr 3, 2020

[7.x] Spaces - Migrate to NP Saved Objects Service (#58716) #62443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spaces - Migrate to NP Saved Objects Service #58716

Spaces - Migrate to NP Saved Objects Service #58716

legrego commented Feb 27, 2020 •

edited

Loading

legrego commented Mar 4, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 2020

legrego Feb 28, 2020

legrego Mar 5, 2020

pgayvallet Mar 9, 2020

legrego Mar 9, 2020

rudolf Mar 16, 2020

legrego Mar 5, 2020

legrego Mar 5, 2020

legrego Mar 5, 2020

legrego Mar 5, 2020

jportner Mar 16, 2020

legrego Mar 5, 2020

elasticmachine commented Mar 6, 2020

legrego commented Mar 24, 2020

legrego commented Apr 1, 2020

legrego commented Apr 1, 2020

legrego commented Apr 2, 2020

legrego commented Apr 2, 2020

legrego commented Apr 3, 2020

kibanamachine commented Apr 3, 2020


		import { deepFreeze } from '../../../../../src/core/utils';

		export const SpacesSavedObjectMappings = deepFreeze({

Spaces - Migrate to NP Saved Objects Service #58716

Spaces - Migrate to NP Saved Objects Service #58716

Conversation

legrego commented Feb 27, 2020 • edited Loading

Summary

legrego commented Mar 4, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 2020

legrego commented Mar 5, 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

elasticmachine commented Mar 6, 2020

legrego commented Mar 24, 2020

legrego commented Apr 1, 2020

legrego commented Apr 1, 2020

legrego commented Apr 2, 2020

legrego commented Apr 2, 2020

legrego commented Apr 3, 2020

kibanamachine commented Apr 3, 2020

💚 Build Succeeded

History

legrego commented Feb 27, 2020 •

edited

Loading