Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove spacesOss plugin #108975

Closed

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Aug 17, 2021

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

@jportner jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Aug 17, 2021
@jportner jportner force-pushed the issue-104152-refactor-spaces-oss branch from 4d07617 to c6cbe45 Compare August 18, 2021 19:09
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!
@jportner jportner force-pushed the issue-104152-refactor-spaces-oss branch from c6cbe45 to 1758d12 Compare August 18, 2021 20:27
mgiota and others added 13 commits August 18, 2021 16:29
…ic#109068)

* [RAC][Observability] remove severity fields from mapping keep only ALERT_SEVERITY

* temporarily remove severity value occurences

* remove ALERT_SEVERITY_VALUE occurences, this value is not being read and shown in the Observability alerts table

* remove duplicate ALERT_SEVERITY identifier

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…c#109122)

* Fix rendering bug on Private Source status

* Fix rendering bug on Private Source status

* Refactor per code review
From 160 to 176px. Prevents overlapping of the labels.

Fixes elastic#109103.
…lastic#108091)

* Adding internal resolve API to resolve rules given an ID

* Updating after merge

* Updating after merge

* Adding resolveRule api to client and adding spacesOss plugin dependency

* Handling 404 errors by calling resolve. Updating unit tests

* Handling aliasMatch and conflict results

* Fixing types

* Unit tests for spaces oss components

* Adding audit event for resolve

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…c#108720)

* [ML] Add tooltip help text

* [ML] Remove unused import

* [ML] Update tooltip content to be clearer

Co-authored-by: Lisa Cawley <lcawley@elastic.co>

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [TSVB] Supports legends with long values

* Add a unit test

* Design optimization

* Revert changes

* Add the missing prop type

* Ensure that the limits are respected

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 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 +3.0B
total -250.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
total -4.3KB
Unknown metric groups

API count

id before after diff
savedObjectsManagement 96 0 -96
spaces 110 198 +88
spacesOss 77 - -77
total -85

API count missing comments

id before after diff
savedObjectsManagement 85 0 -85
spaces 4 16 +12
spacesOss 10 - -10
total -83

References to deprecated APIs

id before after diff
savedObjectsManagement 20 0 -20

History

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

@jportner jportner requested review from a team as code owners August 19, 2021 12:09
Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Author's notes for reviewers

Comment on lines +19 to +40
{ "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" }
Copy link
Contributor Author

@jportner jportner Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 👍

Comment on lines +35 to +46
// 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,
};
}
Copy link
Contributor Author

@jportner jportner Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the most pragmatic approach

Comment on lines -25 to +43
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>());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The CTS flyout can also reuse this data, so we don't need to fetch all spaces every time the flyout is rendered
  2. Other consumers can reuse this data in the future so they don't have to fetch all spaces too

@jportner jportner requested review from spalger and pgayvallet August 19, 2021 12:33
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ML changes LGTM

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maps changes LGTM. Thanks for fixing our tsconfig.json file

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

limits.yml LGTM

@jportner jportner requested review from a team as code owners August 19, 2021 14:40
@jportner jportner requested a review from a team August 19, 2021 14:40
@jportner jportner requested review from a team as code owners August 19, 2021 14:40
@jportner jportner requested review from a team, ashokaditya and parkiino August 19, 2021 14:40
@jportner
Copy link
Contributor Author

Ugh, I merged upstream and it pulled in those changes. Sorry everyone for unnecessary pings, I'll close this PR and open a new one 😫

@jportner jportner closed this Aug 19, 2021
@jportner jportner mentioned this pull request Aug 19, 2021
6 tasks
@jportner jportner deleted the issue-104152-refactor-spaces-oss branch September 14, 2021 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move and refactor x-pack plugin code for Security and Spaces