-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Sharing saved-objects phase 1.5 #75444
Conversation
Resolves conflicts from #75172.
This column should not be shown until we at least have one saved object type that is multi-namespace.
These were unskipped in #74907, but the copy-to-space flyout has changed. Updated the tests to use new test subject selectors, and also updated some of the i18n keys to be more consistent.
docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
Outdated
Show resolved
Hide resolved
docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
Outdated
Show resolved
Hide resolved
docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
Outdated
Show resolved
Hide resolved
docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
Outdated
Show resolved
Hide resolved
docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
Outdated
Show resolved
Hide resolved
Pinging @elastic/ingest-management (Team:Ingest Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking awesome. I have a few edits from reviewing the screenshots:
1/ The label for object id
is on the right of the select component. I think this needs to be switched to the left.
2/ The space between the warning callout and the import json block could use about 16px-24px of spacing to separate them
3/ I think we should try to find a different icon for overwriting compared to new. I'm not sure the color difference will be enough to make it clear. I will work on getting something to you.
4/ Add a cancel option to the Copy to space flyout opposite the copy button
@KOTungseth Thanks for the thorough review. I implemented all of your changes in 3958c02, a couple with slight modifications that I commented on above. I also noticed that you didn't have any feedback for @mdefazio I implemented these changes in 3958c02.
Thanks!
Sure (see below for screenshot). The cancel button is disabled if:
Agree...
Good idea, now that I think of it I'm not sure why there wasn't one to begin with 😅 |
These broke due to the recently merged code for reading request ID from the X-Opaque-Id header in #71019.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reviewed from the PRs targeting the feature branch. Unless anything major changed (in that case, please re-request a review), LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - code review & local testing.
My in-depth reviews were already done in the feature branch, but I have this another round of local testing, and a re-review of the critical components.
Fantastic job, @jportner!! 👏
fyi @ruflin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reposting here from our conversation:
Let's change the summary icons for Error to the alert
icon and the checkInCircle
icon for success. Then I think we can remove the warning color for overwritten.
We will address larger changes for the copy and import summary pages in a separate task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ingest Manager changes* LGTM
*type change in x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts
- type SavedObjectToBe = Required<Omit<SavedObjectsBulkCreateObject, 'version'>> & {
+ type SavedObjectToBe = Required<Pick<SavedObjectsBulkCreateObject, keyof ArchiveAsset>> & {
Kibana logo was used in a few places, changed it to Elastic logo.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these edits! LGTM!
Merging now, will address UI text changes and any additional docs changes in a follow-on PR next week! |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: |
Phase 1.5 from #27004.
Closes #58139
Closes #43873
Closes #48221
Closes #68772
Closes #68958
Overview
This PR makes changes to existing saved object management flows (import, export, copy-to-space) to support multi-namespace (shareable) saved object types. In addition, imports and copy flows have a new option available to "Create new objects with random IDs", which creates a brand new copy of the object. This PR also adds a new user interface for sharing objects.
Changes
Saved Objects Management Table
Added "Shared spaces" column
NOTE: this column is implemented but it is currently disabled. See #69399 (comment) and 10c1cbc for details.
This displays which other space(s) each object is shared to. The display is limited to 5 spaces by default, but it is expandable. Any spaces that a user does not have access to are simply shown as a +X badge.
Two screenshots:
Added "Shared to space" action
This allows users to access the new "Share to space" flyout. It is only enabled for shareable (multi-namespace) saved object types.
Saved Objects Import Flyout
Revamped import options, added "Create new objects" option
Tooltips from top to bottom:
Revamped import options (legacy file)
Added import summary
Before, it simply showed a callout with how many objects it attempted to import (and that number was incorrect due to a bug). New summary after import finishes:
Updated import "Overwrite" modal, added super-select for multiple conflicts scenario
This scenario can happen with the advent of shareable saved objects.
Saved Objects Copy to Space Flyout
Revamped copy options, added "Create new objects" option
In addition, space(s) cannot be selected if an object already exists there; this scenario can happen with the advent of shareable saved objects.
Two screenshots for an object that exists in the current space and in the "bar" space:
Tooltip for spaces cannot be selected:
Revamped copy error resolution / summary
Several screenshots:
Saved Objects Share to Space Flyout
Created flyout
Two screenshots...
Flyout:
Tooltips when an object has been shared and/or unshared:
Docs changes
Preview links for docs changes:
docs/api/saved-objects/find.asciidoc
)docs/api/saved-objects/import.asciidoc
)docs/api/saved-objects/resolve_import_errors.asciidoc
)docs/api/spaces-management/copy_saved_objects.asciidoc
)docs/api/spaces-management/resolve_copy_saved_objects_conflicts.asciidoc
)Testing
Some of these changes cannot be tested with the currently available saved object types. I created a sample plugin that adds a new multi-namespace saved object type and provides a UI to generate some saved objects to test with.