Remove spacesOss plugin #109258

jportner · 2021-08-19T14:59:44Z

Resubmitting #108975.

Partially resolves #104152.

Refactor the SpacesContext provider's fetched space data to make it usable by other consumers
Add CopyToSpaceFlyout to the Spaces plugin's public API contract
Change CopyToSpaceFlyout to work more efficiently by using space data that was loaded by the SpacesContext provider
Remove plugin dependencies on spacesOss plugin (from dashboard and savedObjectsManagement plugins)
- Includes: change savedObjectsManagement plugin to register the Spaces columns/actions directly
Remove spacesOss plugin completely
- Includes: moving all types to spaces plugin
Expose useSpaces function for external consumers to use the Spaces Context provider

This was originally built for only the "share to spaces" components to consume. However, we now have a need for other consumers too. I renamed it to "spacesDataPromise" and I changed the type for each entry, removing the specific "cannotShareToSpace" flag in favor of a generic "isAuthorizedForPurpose" function that can be used for any purpose.

Now uses spacesDataPromise instead of fetching all spaces each time the flyout is opened.

The dashboard and savedObjectsManagement plugins now both depend on the spaces plugin directly!

The maps plugin was missing references, this is just now starting to cause the type checker to fail due to a yet-to-be determined in our TS configuration.

Note: I did this after rebasing onto 3cb3984.

jbudz

limits.yml LGTM

jportner

Author's notes for reviewers. Again.

jportner · 2021-08-19T15:00:19Z

x-pack/plugins/maps/tsconfig.json

+    { "path": "../../../src/plugins/dashboard/tsconfig.json" },
+    { "path": "../../../src/plugins/inspector/tsconfig.json" },
+    { "path": "../../../src/plugins/data/tsconfig.json" },
+    { "path": "../../../src/plugins/ui_actions/tsconfig.json" },
+    { "path": "../../../src/plugins/navigation/tsconfig.json" },
+    { "path": "../../../src/plugins/expressions/tsconfig.json" },
+    { "path": "../../../src/plugins/visualizations/tsconfig.json" },
+    { "path": "../../../src/plugins/embeddable/tsconfig.json" },
+    { "path": "../../../src/plugins/saved_objects/tsconfig.json" },
+    { "path": "../../../src/plugins/share/tsconfig.json" },
+    { "path": "../../../src/plugins/presentation_util/tsconfig.json" },
+    { "path": "../../../src/plugins/home/tsconfig.json" },
+    { "path": "../../../src/plugins/charts/tsconfig.json" },
+    { "path": "../../../src/plugins/usage_collection/tsconfig.json" },
+    { "path": "../../../src/plugins/kibana_react/tsconfig.json" },
+    { "path": "../../../src/plugins/kibana_utils/tsconfig.json" },
    { "path": "../features/tsconfig.json" },
    { "path": "../licensing/tsconfig.json" },
    { "path": "../file_upload/tsconfig.json" },
    { "path": "../saved_objects_tagging/tsconfig.json" },
+    { "path": "../security/tsconfig.json" }


See #108975 (comment):

@elastic/kibana-gis it seems these TS references were mistakenly omitted from your tsconfig.json file. Speaking with @spalger it seems there is a bug in the TS checker which did not surface until my other (unrelated) changes in this PR.

TL;DR these references should have always been here 👍

jportner · 2021-08-19T15:01:43Z

x-pack/plugins/spaces/public/copy_saved_objects_to_space/lib/process_import_response.ts

+// This is derived from the function of the same name in the savedObjectsManagement plugin
+export function processImportResponse(
+  response: SavedObjectsImportResponse
+): ProcessedImportResponse {
+  const { success, errors = [], successResults = [] } = response;
+  const failedImports = errors.map<FailedImport>(({ error, ...obj }) => ({ obj, error }));
+  return {
+    success,
+    failedImports,
+    successfulImports: successResults,
+  };
+}


See #108975 (comment):

I needed to remove this to handle a cyclic dependency between the spaces and savedObjectsManagement plugin.

Since copy-to-space was implemented as a thin veneer over export/import, the CTS flyout used this existing processImportResponse function from the savedObjectsManagement plugin to process the import response.

Turns out we could pare this down quite a bit, we didn't need a lot of what the original function provided for CTS. So I decided to just recreate a CTS-specific function here to remove that problematic dependency 👍

jportner · 2021-08-19T15:02:23Z

x-pack/plugins/spaces/public/spaces_context/wrapper_internal.tsx

-async function getShareToSpacesData(
-  spacesManager: SpacesManager,
-  feature?: string
-): Promise<ShareToSpacesData> {
+async function getSpacesData(spacesManager: SpacesManager, feature?: string): Promise<SpacesData> {
  const spaces = await spacesManager.getSpaces({ includeAuthorizedPurposes: true });
  const activeSpace = await spacesManager.getActiveSpace();
  const spacesMap = spaces
-    .map<ShareToSpaceTarget>(({ authorizedPurposes, disabledFeatures, ...space }) => {
+    .map<SpacesDataEntry>(({ authorizedPurposes, disabledFeatures, ...space }) => {
      const isActiveSpace = space.id === activeSpace.id;
-      const cannotShareToSpace = authorizedPurposes?.shareSavedObjectsIntoSpace === false;
      const isFeatureDisabled = feature !== undefined && disabledFeatures.includes(feature);
      return {
        ...space,
        ...(isActiveSpace && { isActiveSpace }),
-        ...(cannotShareToSpace && { cannotShareToSpace }),
        ...(isFeatureDisabled && { isFeatureDisabled }),
+        isAuthorizedForPurpose: (purpose: GetAllSpacesPurpose) =>
+          // If authorizedPurposes is not present, then Security is disabled; normally in a situation like this we would "fail-secure", but
+          // in this case we are dealing with an abstraction over the client-side UI capabilities. There is no chance for privilege
+          // escalation here, and the correct behavior is that if Security is disabled, the user is implicitly authorized to do everything.
+          authorizedPurposes ? authorizedPurposes[purpose] === true : true,
      };
    })
-    .reduce((acc, cur) => acc.set(cur.id, cur), new Map<string, ShareToSpaceTarget>());
+    .reduce((acc, cur) => acc.set(cur.id, cur), new Map<string, SpacesDataEntry>());


See #108975 (comment):

This function is run when the SpacesContext provider is first mounted. It fetches all spaces and passes them to consumers via the context value.

This was originally purpose-built for the share-to-space flyout, but I renamed it and added a generic isAuthorizedForPurpose function. I have two motivations behind this:

The CTS flyout can also reuse this data, so we don't need to fetch all spaces every time the flyout is rendered

Other consumers can reuse this data in the future so they don't have to fetch all spaces too

YulNaumenko

Alerting changes LGTM!

ThomThomson

Presentation team changes LGTM! It's awesome that we get to simplify the OSS / x-pack relationship now.

jgowdyelastic

ML changes LGTM

nreese

maps changes LGTM

kibanamachine · 2021-08-19T17:18:00Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`savedObjectsManagement`	79	82	+3
`spaces`	240	237	-3
`spacesOss`	4	-	-4
total			-4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`dashboard`	221.4KB	221.3KB	-172.0B
`savedObjectsManagement`	140.8KB	140.7KB	-81.0B
`spaces`	265.2KB	265.2KB	+1.0B
`triggersActionsUi`	1.6MB	1.6MB	-32.0B
total			-284.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`savedObjectsManagement`	34.3KB	39.1KB	+4.8KB
`spaces`	42.3KB	35.8KB	-6.5KB
`spacesOss`	2.5KB	-	-2.5KB
`triggersActionsUi`	89.3KB	89.3KB	-33.0B
total			-4.3KB

Unknown metric groups

API count

id	before	after	diff
`savedObjectsManagement`	96	0	-96
`spaces`	110	203	+93
`spacesOss`	77	-	-77
`triggersActionsUi`	239	238	-1
total			-81

API count missing comments

id	before	after	diff
`savedObjectsManagement`	85	0	-85
`spaces`	4	20	+16
`spacesOss`	10	-	-10
`triggersActionsUi`	230	229	-1
total			-80

Non-exported public API item count

id	before	after	diff
`spaces`	0	1	+1

References to deprecated APIs

id	before	after	diff
`savedObjectsManagement`	20	0	-20

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

pgayvallet · 2021-08-19T22:36:39Z

ack: will review tomorrow

pgayvallet

Thanks for doing this!

pgayvallet · 2021-08-20T06:26:21Z

x-pack/plugins/spaces/public/spaces_context/context.tsx

-const context = createContext<Partial<SpacesReactContextValue<KibanaServices>>>({});
+const context = createContext<Partial<SpacesReactContextValue<Partial<CoreStart>>>>({});


(not related to this PR, was already there: Why was the spaces context extending KibanaServices, and why do we need to have it now extends Partial<CoreStart>? Why is SpacesReactContextValue extending anything?

I based this implementation off of the Kibana React Context:

kibana/src/plugins/kibana_react/public/context/context.tsx

Line 22 in 4584a8b

export const context = createContext<KibanaReactContextValue<KibanaServices>>(defaultContextValue);

kibana/src/plugins/kibana_react/public/context/types.ts

Line 14 in 4584a8b

export type KibanaServices = Partial<CoreStart>;

The useSpaces context also provides access to core start services so you don't need to use two different context providers in the client consumer. I guess it was written with this generic type so you can specify what core services you need to use.

pgayvallet · 2021-08-20T06:47:14Z

x-pack/plugins/spaces/public/ui_api/index.ts

 export const getUiApi = ({ spacesManager, getStartServices }: GetUiApiOptions): SpacesApiUi => {
  const components = getComponents({ spacesManager, getStartServices });

  return {
    components,
    redirectLegacyUrl: createRedirectLegacyUrl(getStartServices),
+    useSpaces,


Thanks for this addition.

However, the useSpaces hook will only work for functional components (not class components). Class components will need to either have a reference of the context class to use the contextType property, or have access to the context.Consumer component (and most of the 'legacy' 'controller' components are class-based).

Any objection adding a getSpacesContextConsumer API to SpacesApiUiComponent? If that's fine, I could do it directly in #109196

Sure that's fine, thanks for adding it!

azasypkin · 2021-08-23T09:09:04Z

ACK: reviewing...

azasypkin

Looks great, thanks!

azasypkin · 2021-08-23T11:39:14Z

api_docs/spaces_oss.json

@@ -1,1320 +0,0 @@
-{


question: do you know why we still mention spacesOss in spaces.json (and in triggers_actions_ui.json)?

Yeah this was intentional. We don't automatically require regenerating the new API docs for each PR (yet); whenever we next regenerate them that will go away.

Ah, nice, thanks

azasypkin · 2021-08-23T11:56:12Z

src/plugins/saved_objects_management/public/services/column_service.test.ts

+      const start = service.start(spacesPluginMock.createStartContract());
+      expect(start.getAll()).toEqual([
+        column,
+        // expect.any(ShareToSpaceSavedObjectsManagementColumn),


nit: You left a clarifying comment in column_service.ts, would you mind leaving a brief note why we keep commented code here as well?

Ah, auto-merge already happened when the PR was approved 😄 I'll make a mental note to do this next time I'm in the code.
FWIW the existing test will fail if the code in column_service.ts is uncommented, and this needs to be uncommented to make it pass.

azasypkin · 2021-08-23T12:38:47Z

x-pack/plugins/spaces/public/copy_saved_objects_to_space/lib/process_import_response.ts

+export interface ProcessedImportResponse {
+  success: boolean;
+  failedImports: FailedImport[];
+  successfulImports: SavedObjectsImportSuccess[];
+}


note: type names feel a bit inconsistent:

failedImports - FailedImport[]

successfulImports - SavedObjectsImportSuccess[]

Why not SuccessfullImport[] (or SavedObjectImportFailure[])?

Yeah, SavedObjectsImportFailure and SavedObjectsImportSuccess both come from Core.
SavedObjectsImportFailure is older and commingles the error with the rest of the object fields. So basically the entire point of this function is to "un-commingle" that into FailedImport 😅.

I added SavedObjectsImportSuccess later, but I was able to use it on the client side without any additional processing.

I can add this:

export type SuccessfulImport = FailedImport;

next time I'm in the code, agree it would make things clearer.

Thanks for the explanation!

kibanamachine · 2021-08-23T13:13:43Z

💔 Backport failed

Status	Branch	Result
❌	7.x	Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 109258

# Conflicts: # .eslintrc.js # .github/CODEOWNERS # src/plugins/dashboard/kibana.json

jportner added 9 commits August 19, 2021 10:53

Add CopyToSpaceFlyout to spaces public API contract

c417313

Change CopyToSpaceFlyout to use data from SpacesContext provider

33bd772

Now uses spacesDataPromise instead of fetching all spaces each time the flyout is opened.

Remove dependencies on spacesOss plugin

6268fbc

The dashboard and savedObjectsManagement plugins now both depend on the spaces plugin directly!

Remove spacesOss plugin

d835a4d

Remove spaces dependency on savedObjectsManagement

a862b12

Fix TS references

207978a

The maps plugin was missing references, this is just now starting to cause the type checker to fail due to a yet-to-be determined in our TS configuration.

Remove spacesOss dependency from triggers_actions_ui plugin

ce4ded6

Note: I did this after rebasing onto 3cb3984.

Expose useSpaces function to external consumers

f165b5b

jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Aug 19, 2021

jportner requested review from a team as code owners August 19, 2021 14:59

jbudz approved these changes Aug 19, 2021

View reviewed changes

jportner commented Aug 19, 2021

View reviewed changes

YulNaumenko approved these changes Aug 19, 2021

View reviewed changes

jportner requested review from spalger and pgayvallet August 19, 2021 15:04

ThomThomson approved these changes Aug 19, 2021

View reviewed changes

jgowdyelastic approved these changes Aug 19, 2021

View reviewed changes

jportner requested review from a team and removed request for spalger August 19, 2021 15:05

jgowdyelastic approved these changes Aug 19, 2021

View reviewed changes

nreese approved these changes Aug 19, 2021

View reviewed changes

jportner added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 19, 2021

jportner enabled auto-merge (squash) August 19, 2021 20:31

pgayvallet approved these changes Aug 20, 2021

View reviewed changes

azasypkin self-requested a review August 23, 2021 09:09

azasypkin approved these changes Aug 23, 2021

View reviewed changes

jportner merged commit 65e04b1 into elastic:master Aug 23, 2021

jportner mentioned this pull request Aug 23, 2021

[7.x] Remove spacesOss plugin (#109258) #109649

Merged

jportner added a commit to jportner/kibana that referenced this pull request Aug 23, 2021

Remove spacesOss plugin (elastic#109258)

bf349db

# Conflicts: # .eslintrc.js # .github/CODEOWNERS # src/plugins/dashboard/kibana.json

jportner deleted the issue-104152-refactor-spaces-oss-AGAIN branch August 23, 2021 13:53

jportner added a commit that referenced this pull request Aug 23, 2021

Remove spacesOss plugin (#109258) (#109649)

492de4e

# Conflicts: # .eslintrc.js # .github/CODEOWNERS # src/plugins/dashboard/kibana.json

This was referenced Aug 24, 2021

Add multi-space support to SO management and import/export API #109196

Closed

Add multi-namespace capabilities to SOR.bulkGet #109197

Closed

jportner mentioned this pull request Oct 6, 2021

[meta] Improve deprecation warnings for plugins without an enabled config in 8.0. #114188

Closed

100 tasks

jportner mentioned this pull request Oct 28, 2021

Copy saved object flyout should not allow copying into the active space #116650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove spacesOss plugin #109258

Remove spacesOss plugin #109258

jportner commented Aug 19, 2021

jbudz left a comment

jportner left a comment

jportner Aug 19, 2021

jportner Aug 19, 2021

jportner Aug 19, 2021

YulNaumenko left a comment

ThomThomson left a comment

jgowdyelastic left a comment

nreese left a comment

kibanamachine commented Aug 19, 2021

API count

API count missing comments

Non-exported public API item count

References to deprecated APIs

pgayvallet commented Aug 19, 2021

pgayvallet left a comment

pgayvallet Aug 20, 2021

jportner Aug 20, 2021

pgayvallet Aug 20, 2021 •

edited

Loading

jportner Aug 20, 2021

azasypkin commented Aug 23, 2021

azasypkin left a comment

azasypkin Aug 23, 2021

jportner Aug 23, 2021

azasypkin Aug 23, 2021

azasypkin Aug 23, 2021

jportner Aug 23, 2021

azasypkin Aug 23, 2021

jportner Aug 23, 2021 •

edited

Loading

azasypkin Aug 23, 2021

kibanamachine commented Aug 23, 2021

		const context = createContext<Partial<SpacesReactContextValue<KibanaServices>>>({});
		const context = createContext<Partial<SpacesReactContextValue<Partial<CoreStart>>>>({});

Remove spacesOss plugin #109258

Remove spacesOss plugin #109258

Conversation

jportner commented Aug 19, 2021

jbudz left a comment

Choose a reason for hiding this comment

jportner left a 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

YulNaumenko left a comment

Choose a reason for hiding this comment

ThomThomson left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 19, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

API count

API count missing comments

Non-exported public API item count

References to deprecated APIs

pgayvallet commented Aug 19, 2021

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Aug 23, 2021

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

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

jportner Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 23, 2021

💔 Backport failed

pgayvallet Aug 20, 2021 •

edited

Loading

jportner Aug 23, 2021 •

edited

Loading